Timed iteration updates and backward compatibility standards


#21

The stan/services/util/generate_transitions.hpp file does not include the chain ID in its stringstream:


If you want me to add it in RStan or other interafaces, I can do that, but I don’t think interfaces should be responsible for doing things like that.


#22

I agree. That’s why we’re trying to move all this down to the services layer.


#23

I am definitely open to different ideas about how to do time-based refreshes. I just don’t think adding a refresh_seconds argument that preserves the existing behavior moves the needle very much on accomplishing the two goals of

  1. Printing less crap on the screen, particularly when it has negative value added in short runs
  2. Making it easier to guestimate “How much longer will this long run take?”

Over months or more realistically years, users would learn to specify the refresh_seconds argument, but in the interim you still have all the costs I mentioned. In contrast, reinterpreting refresh makes immediate progress toward those two goals with not much cost.

No, I mean ones like those in rstanarm. If you have a compiled version of foo.stan by CmdStan, then ./foo.stan will still work (albeit with the old progress behavior) and it is pretty easy for the CmdStan user to recompile foo.stan in order to get the new progress behavior. Someone who is using an R package with old compiled models that do not have a refresh_seconds argument and a new RStan that is passing a refresh_seconds argument gets an error message that they can’t do anything about until the R package gets recompiled with newly generated C++.


#24

I take it that’s because all of our services template the model class? We really need to change this so it’s not a problem going forward. That is, all the services should take references to a base class, not a template. Then we can also speed up compilation.

Cleaning up the base class for models and making this change is the number one priority for me after MPI goes in and @mitzimorris’s refactor of the AST lands. So we should have a solution to this in 2.19, I hope. I really want to start tackling compilation time problems, and this will help with that.


#25

This is exactly what the interfaces are responsible for. See:

It’s just a matter of configuring the writer properly (which the code is already there for it to happen). CmdStan doesn’t need to do this because it doesn’t work with multiple threads.

Just add the appropriate prefix to the current writer and it’ll get fixed. I thought I submitted a PR to fix it, but maybe I didn’t.


#26

The main thing is for the chain ID to get restored one way or another. But why is it the responsibility of stan/services/util/generate_transitions.hpp to report the Iteration, the percentage of iterations completed, whether it is during or after the warmup period, and the setw but it is the responsibility of the interfaces to report the chain ID?


#27

It makes sense from the stan-dev/stan perspective because each service method is launching one chain, so of course the interface has to tell the code which chain is which at writer construction. We could change services to handle multiple chains but that’s another story… it would be really easy to do with threads with zero impact on performance at this point since threading was already introduced within chains.


#28

I agree about threads. In the meantime, it is probably easiest to add the chain ID in the interfaces. But the services API has to know the chain ID in order to know which file to write to, so it should be able to construct a complete message and just say to the interfaces “print this”.


#29

BTW: My preference is to just keep ‘refresh’ or whatever the argument is called and give it the new meaning. We all seem to agree there’s minimal cost and its an aspect of services that downstream should NOT rely on. It will mess up some scripts somewhere a little, in cosmetic terms, but the results will be fine. Thanks for putting the PR together.

My position is that the services should produce something like ‘chain id’, ‘iterations’, etc… and we should provide some functions within stan-dev/stan that can format these into standard messages. That way we could say both “you can rely on the presence of a chain ID and iteration output as an integer” and “you can’t rely on the exact text of the message so if you do good luck keeping up”.


#30

That is plausible, but we should have some standards to keep what the interfaces print consistent.


#31

Makes sense, I’m imagining that the helper functions should be used to achieve this goal and have well-documented standard usage.


#32

Hi, (sorry if this is off-topic)

In PyStan 3 the sampling progress output is done with tqdm-progressbar library which has support for multiple progress bars and updates everytime a draw is sampled (httpstan is streaming output from Stan model).

Something similar probably could be done with CmdStan / RStan interfaces.

edit. cc @ariddell


#33

The issue is whether the interfaces or stan should be responsible for things like refresh and progress updates. The motivation for pulling things down into C++ implementations in Stan is that it enforces uniformity for the interfaces.

PyStan3 really has multiple interfaces if I understand what you’re trying to do:

  • the server interface to Stan
  • the client interface to the server
  • the user interface to the client

#34

Thanks @bgoodri for the clear justifications. I now agree with you.


#35

R has also a nice progress bar package which works also in terminal.

I would say interfaces, but they should have easy way to access the current progress status without need to read csv. But I’m fine with Ben’s proposal now, as it probably takes some time before interfaces could get that information easily.


#36

I think there’s a better way to do this, but happy to move forward with this change.

My request is that it’s documented correctly. The refresh in seconds is not a guarantee that a message will be written at that time. It means that if you wanted for a refresh of 60 seconds and it took 60,000 seconds to run, you won’t get 1000 messages.

The better way to do this is just to notify when there’s an iteration (this is what I’ve suggested). Then the interfaces could format the message consistently with functions in Stan, but would have the ability to run a separate progress bar or something in a different thread to handle the user’s request of refreshing at a specific time much more easily. And this behavior that’s requested could also be handled easily.

Actually, it wouldn’t be hard to do this. @bgoodri, are you in a rush? Can this wait for the weekend?


#37

Isn’t that currently what happens with the writers?


#38

No. And that’s why I think we can just do this a lot better.

What’s getting output is a string message that looks like:

Iteration: 2000 / 2000 [100%]  (Sampling)

The logic as to when that gets printed is handled by the service code. I think it’s just a lot cleaner for the services to let the client know that there’s another iteration (instead of jamming a string into a something that’s supposed to be written out) and have the client deal with it how it wants. If the client wants to handle a real timer in a separate thread so that it’s actually reported every X seconds, that’s fine. If the client wants to use a progress bar, that’s fine too.

Sure, we can change this argument, but I think it just falls short. It’s really unsatisfying to me that it’s not actually going to report the iteration number at the specified time. For example, if the requested seconds was 30 and each iteration lasts 600 seconds, then I think it’ll just look broken.


#39

That’s a good point.

The other place I wonder it’ll look broken is when we have something like refresh = 10 and that used to spew output and now it waits 10 seconds.

We’re trying to balance three demands:

  1. wanting the interfaces to behave the same way,
  2. not wanting to write code multiple times,
  3. giving the interfaces flexiblity to do whatever they want with the output.

I think @syclik is arguing we want to prioritize (3) over (1) and (2).

I don’t see how this is going to prevent progress bars. Those would go on the iterations, not this refresh thing that’s just a message to the console.

As far as messages to the console go, I thought we were OK with having the basic logging/console output be not under the same backward compatiblity requirements as draws, etc. If that’s not everyone’s understanding we need to come to some common understanding so we can make progress.


#40

I am trying to fix this for 2.18 but I don’t understand. Before we were passing strings around, so you could prefix. Now, it just passes this stream_logger thing


to Stan and Stan builds the info message.