cmdstanr icon indicating copy to clipboard operation
cmdstanr copied to clipboard

Background/asynchronous sampling (feature request & design discussion)

Open mike-lawrence opened this issue 3 years ago • 11 comments

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)

mike-lawrence avatar Jan 02 '21 22:01 mike-lawrence

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.

rok-cesnovar avatar Jan 03 '21 16:01 rok-cesnovar

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?

mike-lawrence avatar Jan 03 '21 18:01 mike-lawrence

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.

rok-cesnovar avatar Jan 03 '21 18:01 rok-cesnovar

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.

mike-lawrence avatar Jan 03 '21 18:01 mike-lawrence

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.

rok-cesnovar avatar Jan 03 '21 18:01 rok-cesnovar

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).

mike-lawrence avatar Jan 03 '21 21:01 mike-lawrence

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 sample() in the bbr package I'm working on and I want to enable two things:

  • primarily I want to be able to launch a model in the background and check on it periodically (by looking at the proposed log file)
  • Once I can do this first thing, I'd eventually like to launch a model (or potentially multiple models) to run in the background, which is basically the original point of this issue. I was considering rolling-my-own by using callr or something, but obviously it would be better to have an officially supported way of doing this.

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.

seth127 avatar Mar 15 '21 19:03 seth127

I agree that custom logs would be nice for cmdstanr, but I am less sure about async. With the latter, it feels like we may be getting into workflow territory beyond the scope of the package (but clearly the maintainers are the judge of that).

Personally, I invoke cmdstanr through stantargets. It already has logging and async workarounds I like, so I do not plan to personally contribute similar features to cmdstanr.

bbr looks really cool though, super comprehensive.

wlandau avatar Mar 16 '21 01:03 wlandau

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 cmdstanr being able to pipe output to a log instead of the console.

@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?

seth127 avatar Mar 17 '21 16:03 seth127

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.

rok-cesnovar avatar Mar 17 '21 17:03 rok-cesnovar

@seth127, stantargets uses withr::local_output_sink() and withr::local_message_sink() to manage logs. I still think a solution in cmdstanr would be ideal.

wlandau avatar Mar 18 '21 02:03 wlandau