-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Functionality to Restart R Within R-Instat #9319
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@N-thony this seems to work fine. I'm approving and expecting @Patowhiz to also check.
I tried to look at why you might want to do this and it seems that it largely to do with the way R uses memory.
This nicely completes the main tasks in the Tools menu. Should we now just get rid of the Run R Code and Save Current Options items? That could be a separate pull request.
@rdstern this is just a draft, I'm looking forward to replicate the RStudio restart system in R-Instat. The memory usage is increasing in this feature because this is not fully implemented. What's Missing
Once this is done, I think we will have a nice Restart system. |
@rdstern, I think this will work efficiently especially once we start using R-Instat packages developed by Lily. |
@ChrisMarsh82 could you please also look at this one? It mirrors a facility in RStudio and we have asumed it in the Tools menu from the start. And maybe merge once you are ok? |
@ChrisMarsh82 line 2957 in main |
@N-thony @ChrisMarsh82 has added a very brief message. Is this locating the source of the conflicting files? So back to you. |
@rdstern we had a discussion about this Me and @ChrisMarsh82 who will review it and get back to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see a way to get the memory to drop to origianl, although the changes do drop the memory back to close to where it was. Over a long period it seems to keep the memory consistent. I would suggest adding the forceGarbageCollection.
I looked at the threads and there is a possible memory leak in evaluate however this seems to reset when the procedure runs again so I don't think it is worth making changes here.
Is there something I am missing in your tests can you get the memory to keep going up?
|
||
' Reset initialization flag | ||
bREngineInitialised = False | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clsEngine.ForceGarbageCollection()
This doesn't seem to make too much difference to memory but is probably good to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisMarsh82 I was tracking the memory usage with pryr::mem_used()
function in the log file. As you can see for the first time I click on restart, the memory usage goes down from 131.58 MB
to 126.34 MB
then we need to reset the working folder which makes go up to 198.48 MB
. My understanding is with current implementation, the restarting feature makes the memory usage increase. I was hoping with the use of our instat package from @lilyclements, we won't need setting up the working directory anymore which will make this feature efficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisMarsh82 and @N-thony I like the discussion above. When I first ran it crashed out, saying pryr wasn't installed?
I assume it will be automatically in the new version? After installing it ran nicely.
I am happy to approve. This (I think) now concludes the improvements we plan to make in the Tools menu? So the remaining item here - or in a new issue - is to delete the 2 disabled items in that menu. Namely Run R code, and save current options.
@ChrisMarsh82 I am hoping that we can merge some open pull requests tomorrow (Wenesday), ahead of merging @lilyclements implementation of the R-Packages. Is this one you can approve, or should it wait? |
Moving to instat calculation
@rdstern now we are using our four packages, I'm facing some challenges and made me think how this can be improve now. I will probably open a new PR and close this one soon. |
Fixes partially #9308
@rdstern @jkmusyoka, this is a draft implementation of the functionality for restarting R within R-Instat. Please review and share your feedback.