-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Background/asynchronous sampling (feature request & design discussion) #424
Comments
I agree with supporting non-blocking sampling and other algorithms. I think we should start with supporting non-blocking that finishes while the session is still active. Meaning that if you close R, the processes would be stopped. That does not require storing stdout/stderr in files, we can still rely on processx. That also simplifies all the other code as the main change is that we do check stdout/stderr and wait for processes to finish. |
Hm, this part lost me; would you mind clarifying? |
Sorry for the confusing language. If we go with non-blocking calls (background) but require that the user does not close the session we do not need to add much to what we do now. In that case we just need to change that we do not process stdout/stderr while executing the processes and that we do not wait for the processes to finish. The rest of it mostly stays the same. |
Ok, gotcha. And we will be processing the stdout/stderr, just in a separate function that is called at-will by the user. Oh, wait! I was reading the processx docs the other day and they warn that it's possible for the pipes it creates for stdout/stderr to fill their buffers if the user doesn't check them often enough. As I understand it, diverting stdout/stderr to files doesn't have the same limitation. |
That is a good point. In case of doing it in session these can be temp files. |
Oh, as an aside: using files for stdout/stderr should make it easier for users to enforce silent running. I've personally found it irksome how difficult it is to to eliminate all output during sampling, resorting to sink('/dev/null') and even then still getting the end-of-sampling diagnostic warnings popping up if they occur. (I know those messages are important, but I normally run diagnostics checks separately anyway, so the ones that pop up unbidden are superfluous). |
Hello. I wanted to see if there was any progress on this. I was about to put in an issue requesting that it be possible to route all of the messages and output to a file instead of the console, but then I noticed this issue which discusses the same thing (and more). And then also #429 which mentions the same thing. I see @wlandau put in a PR for a quick fix on 429. Are you considering this "piping to log file" change (that he mentions here) any time soon? If not, would you look at a PR for optionally routing messages to a file? Not sure when I could get to it, but if it's not on your radar for any time soon then I would try to make some time (and/or see if @wlandau was interested). To be clear on my intentions: I'm wrapping
Ok, thanks for consideration. Let me know if you want me open another issue specifically about sending the messages to a file instead of the console, and if you're interested in a PR for that as well. |
I agree that custom logs would be nice for Personally, I invoke
|
thanks @wlandau. I'm interested in how you are capturing the output (the "logging workaround" you describe). I guess maybe this isn't the place for that discussion, but I wanted to clarify whether you are still interested in @rok-cesnovar thanks for adding this to the milestone. Do you want me to make a more concise issue about the logging or are you happy with everything contained in this issue? |
Please do, just so we dont forget. Maybe propose argument names as well. Lets keep this one about async cmdstan execution. |
@seth127, |
I propose modification of the various run methods to enable doing so in the background rather than occupying the foreground R session. This is useful for scenarios where one wants to do other things without launching a new session, as well as to make runs robust to crashes of the foreground R session.
To achieve this, I propose separating out the initiation and polling of the processx processes.
To enable continued running amidst crashes of the foreground R session, I propose starting the processes with cleanup=FALSE and saving the PID in an RDS file so that ps can be used to resume management of the process. This also requires that files be used for stdout and stderr (rather than the processx default of using pollable pipes back to the R session) so that monitoring progress can resume by simply checking these files. This latter in turn calls for a change in the cmdstanr approach to polling from using the build-in processx polling to polling based on checking/parsing the stdout/stderr files (using mod time or filesize to inform whether to bother looking for newlines).
While not necessary for the above, I propose storing these files in a folder called
stan_scratch
(whose default-but-configurable location is the current working dir). (n.b. said folder is involved in the proposed implementations of these FRs as well: During-sampling diagnostics, Recompile only on changes to output of stanc3 auto-formatter )Similarly, I propose starting a separate process for each chain (in parallel if parallel chains are requested). (I'm pretty sure that's what cmdstanr does already, but wanted to be explicit here)
The text was updated successfully, but these errors were encountered: