New message on STAN_THREADS

[wds15]

Hi!

I just saw that in the current develop version I am getting a message like

STAN_THREADS is enabled. map_rect will run with at most 6 thread(s).

I like the idea behind this… but what I would suggest is

• do not quote a compile time STAN_THREADS flags
• we specifically mention map_rect…which we should not as we will use more things with threads

So my suggestion would be to rephrase this to

number of threads used for this chain: XX

or something similar.

I am not sure if this is a parser or a cmdstan matter.

Sebastian

Tagging @rok_cesnovar and @seantalts

[rok_cesnovar]

I added this to cmdstan awhile ago. I do realize my mistake for not at least tagging you if you are cool with the change. Sorry for that.

I added the same thing for OpenCL to display which device is being used. There were some support questions related to users wanting to be sure that either threads or OpenCL (and which device) are being used. So that was the reasoning behind this change.

@wds15

• do not quote a compile time STAN_THREADS flags

I do agree that maybe mentioning the flag itself was not my best idea. “Number of threads used …” would be better. I was specifically mentioning the flag because this message will not appear if the flag is not set. Once we have threads on by default, this will be easier to handle of course.

@wds15

• we specifically mention map_rect …which we should not as we will use more things with threads

I dont really agree with this one, For now, threading is used for map_rect. Once we extend the functionality of threading we can change the message to not specifically mention map_rect. That is a simple change.

If the users will see messages like “using 4 threads” they will expect speedups that will not be there if no map_rect is used. I know that will all change, hopefully sooner rather than later.

@wds15

I am not sure if this is a parser or a cmdstan matter.

The changes were made here: https://github.com/stan-dev/cmdstan/blob/develop/src/cmdstan/command.hpp#L137

[wds15]

@rok_cesnovar

I added this to cmdstan awhile ago. I do realize my mistake for not at least tagging you if you are cool with the change. Sorry for that.

No problem.

@rok_cesnovar

I dont really agree with this one, For now, threading is used for map_rect . Once we extend the functionality of threading we can change the message to not specifically mention map_rect. That is a simple change.

I see your motivation here, but I still think that we should not easily change the printout from our programs so easily - and we know that we will have more utilities that will use threading. So in hindsight of what we know is coming, I think the message should avoid mentioning map_rect specifically. The better behaviour would be to give a warning at the moment if threading us turned on, multiple threads are requested, but no map_rect is found in the program (but that’s hard to code up I suppose). Maybe the parser can detect if parallelism is used and then we can create a message based on that info?

A few more thoughts:

• I actually think that this statement should be part of the stan services which do printout of the chain run parameters. I mean, the exact numerical results will depend on the choice of the number of threads.
• In case the user turns on threading and MPI, then MPI will be used for parallelism (and only a nested map_rect would use threads). Turning on both is not recommended, but it should work.
• At the moment we always report the “elapsed time”… which is not quite true when threading is used as the displayed time is the CPU time used, but not the wall time used. That needs to change in the stan services. We should report what we say and what users care about - and that is wall clock time.

If everyone is fine with changing the cmdstan printouts easily and do not make these subject to our usually very rigorous deprecation policies, then we can sort this out with some time.

[rok_cesnovar]

@wds15

The better behaviour would be to give a warning at the moment if threading us turned on, multiple threads are requested, but no map_rect is found in the program (but that’s hard to code up I suppose).

Yeah, that requires interaction with stanc. It should not be hard, but its not trivial either. And certainly something that we would have to change after the release, not before.

@wds15

I actually think that this statement should be part of the stan services which do printout of the chain run parameters. I mean, the exact numerical results will depend on the choice of the number of threads.

Seems reasonable. I opened an issue to modify or revert this change before the release. Lets discuss the details there so they dont get lost or burried on Discourse.

@wds15

In case the user turns on threading and MPI, then MPI will be used for parallelism (and only a nested map_rect would use threads). Turning on both is not recommended, but it should work.

Good point. That case does need to be handled if we leave this in.

@wds15

At the moment we always report the “elapsed time”… which is not quite true when threading is used as the displayed time is the CPU time used, but not the wall time used. That needs to change in the stan services. We should report what we say and what users care about - and that is wall clock time.

That is a separate issue, but yes, I fully agree with this. In cmdstanr we resorted to timing the execution time in R for this exact reason.

[Bob_Carpenter]

@wds15

I still think that we should not easily change the printout from our programs so easily

@wds15

If everyone is fine with changing the cmdstan printouts easily and do not make these subject to our usually very rigorous deprecation policies, then we can sort this out with some time.

We decided a while back in a long discussion with @seantalts and @bgoodri that console output wasn’t considered something we needed to preserve for backward compatibility.

To me, the biggest risk to the stability of the project going forward is (a) the new parser, and (b) threading.