Simplify return codes; error messages written to stderr, excepting model instantiation failures #994
Conversation
|
@syclik - any thoughts w/r/t return codes? |
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
src/cmdstan/command.hpp
Outdated
| @@ -219,7 +223,7 @@ int command(int argc, const char *argv[]) { | |||
|
|
|||
| // Instantiate model | |||
| stan::model::model_base &model | |||
| = new_model(*var_context, random_seed, &std::cout); | |||
| = new_model(*var_context, random_seed, &std::cerr); | |||
There was a problem hiding this comment.
Will this make print statements in the Stan model go to stderr?
There was a problem hiding this comment.
excellent question. will check.
There was a problem hiding this comment.
answer is no. the generated model.hpp file contains the function new_model; this instantiates the model object.
the instantiated model object is passed into the services methods along with stderr and stdout, and therefore the correct output streams are hooked up to the calls to log_prob et al.
can't connect the c++ dots to understand why calls to rethrow_located in the model constructor go to stderr, but they do.
There was a problem hiding this comment.
print statements in transformed data block will go to std err, because those are rolled into the model constructor. everything else goes to stdout.
There was a problem hiding this comment.
functions that print stuff will print to stderr when called in transformed data block, otherwise, will print to stdout.
There was a problem hiding this comment.
Sounds like this is a cleanup for stanc3. Since it's only transformed data prints, then I think revert this change and just do the return code cleanup here.
There was a problem hiding this comment.
the model instantiation failures caught by validate_dims are printed to whatever output stream is passed in to new_model. I spent an hour yesterday trying to figure out why - I don't see an output stream passed around, and yet, that's where these error messages end up.
the result it that it's necessary to parse stdout to figure out if the data supplied to model is OK. cf https://github.com/stan-dev/cmdstanpy/blob/2a7a9a5cc3a8e37670092c0af11d1e4c6d252381/cmdstanpy/stanfit.py#L230-L244. - this is a quick and dirty parse - it's brittle, might catch too much/too little, etc.
just to be clear, if the user has a print statement in transformed data block, it will go to stderr; otherwise, stdout.
but yes, agree, this is terrible either way.
There was a problem hiding this comment.
Hmm I guess we can go ahead and fix this streaming thing then.
I was looking at the generated cpp for a model:
stan::model::model_base& new_model(
stan::io::var_context& data_context,
unsigned int seed,
std::ostream* msg_stream) {
stan_model* m = new stan_model(data_context, seed, msg_stream);
return *m;
}Then I went to look at the stan_model constructor in my generated code and the pstream__ argument is unused there. Where does this argument come in to play anyway?
There was a problem hiding this comment.
Or I guess the question is how does this argument end up changing where stuff goes anyway? Is this a problem with how some service function is not using loggers where it should be or something weird?
There was a problem hiding this comment.
I'd love to figure out what's going on here and then address it in a future PR.
for now, can let thing go to stdout for now and just simplify return codes with this PR.
in theory, sounds like a good idea - cf this nice writeup of why it's better to throw exceptions rather than use return codes: http://javierferrer.me/exceptions-vs-error-codes/ but we can't do this (yet) - my understanding is that RStan and PyStan call the services methods directly and they couldn't handle C++ calls throwing exceptions, so we went this way. maybe @syclik can explain more. |
…v/cmdstan into feature/991-improve-errs
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
mitzimorris
changed the title
|
changed title to match outcome of discussion. |
I believe RStan and PyStan can handle the exceptions! I agree that it makes more sense if they throw appropriate exceptions and let each interface handle error handling. In C++, if you don't handle an exception, it'll eventually crash the process. I'm not sure if it's well defined what the outcome is... it's better if we turned that exception into something reasonable (and returned a non-zero exit code). |
when an uncaught exception causes the process to crash, depending on what the error is, the system returns a termination signal which is platform dependent, c.f this table: https://en.wikipedia.org/wiki/Signal_(IPC)#Default_action understood that the calling process needs to handle the exception - this is always the case.
this writeup corresponds to my experience with CmdStan - the slim set of Posix return codes weren't useful in providing feedback to the user. |
Thanks! Sorry about the language there... didn't mean to imply that you didn't understand that. I was actually wondering what would happen if CmdStan didn't catch the exception and whether it'd be well-defined. Just from the perspective of what happens if we let it propagate (simplifies code)... I learned something new from your response... I didn't realize an uncaught exception results in a SIGTERM! I hadn't found that info in my searching around for what the behavior is. |
|
I'm definitely in favor of letting service methods throw errors instead of return codes. |
Submisison Checklist
./runCmdStanTests.py src/testSummary:
Ensures that all error messages are written to stderr, including bad input data.
PR #992 overlooked the output stream passed in to the
new_modelfunction; changed it from standard output to standard error, as the only messages written to this output stream are error messages.Intended Effect:
Consistent handling of error messages - all error messages should go to standard error.
Simplify return codes - the set of POSIX return codes in stan::services::error_codes aren't actionable for CmdStan users and the interfaces which wrap them.
How to Verify:
Unit tests.
Side Effects:
As for #992, document that errors are not going to standard error, not standard out.
Documentation:
CmdStan User's guide now has section on return codes and common errors.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Columbia University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: