Skip to content
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

Only swallow domain_errors in various algorithms #3259

Merged
merged 6 commits into from
Feb 13, 2024

Conversation

WardBrian
Copy link
Member

Submission Checklist

  • Run unit tests: ./runTests.py src/test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary

Closes #3258. This changes any place which caught std::exception from the model to only catch std::domain_error, matching what is done during initialization.

This prevents odd behavior like an indexing exception that only occurs randomly being swallowed if initialization happens to pass, and allows things like an exit() function in the language.

Intended Effect

Cause certain exceptions to terminate the inference algorithms.

How to Verify

This model will usually initialize but quickly fail now:

parameters {
  real mu;
}
model {
  mu ~ normal(0, 1);

  if (mu > 1) {
    array[1] int sigma;
    print(sigma[2]);
  }
}

Side Effects

I've installed handlers in the service functions, but code that calls the internals of these algorithms (e.g run_adaptive_sampler) not through the services will need to be ready to catch exceptions

Additionally, if any Math functions raise an exception which is intended to be recoverable but is not a domain_error, they will need updating. Because this is the current behavior in initialization, I suspect we have probably already smoked any of these out.

Documentation

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

Simons Foundation

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@WardBrian

This comment was marked as outdated.

@WardBrian
Copy link
Member Author

@SteveBronder mind giving this a look?

@WardBrian WardBrian force-pushed the fix/3258-unconditional-exception-swallowing branch from de17945 to 759a569 Compare February 8, 2024 21:47
Copy link
Collaborator

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, one thing I would like clarified is what happens to things like std::out_of_range errors.

Comment on lines 131 to 136
try {
ret = bfgs.step();
} catch (const std::exception& e) {
logger.error(e.what());
return error_codes::SOFTWARE;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to the outer part of the while loop?

Comment on lines 126 to 131
try {
ret = lbfgs.step();
} catch (const std::exception& e) {
logger.error(e.what());
return error_codes::SOFTWARE;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Generally for any while loops I'd like to just have the try on the outside

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved both

src/stan/services/pathfinder/multi.hpp Show resolved Hide resolved
Comment on lines +824 to +826
// we want to terminate multi-path pathfinder during these unrecoverable
// exceptions
throw;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? One pathfinder can fail while the others succeed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one path does something unrecoverable like indexes into a vector out of bounds, I think the right thing to do is stop and say "something is clearly wrong with your model".

Besides that, a primary motivation behind this PR is also to implement an exit() function into the language, which requires the ability to terminate the entire algorithm it is in. If one chain or one path calls exit, the entire thing still needs to halt.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one path does something unrecoverable like indexes into a vector out of bounds, I think the right thing to do is stop and say "something is clearly wrong with your model".

Are only unrecoverable errors able to be thrown here? Like if the call to log_prob throws it could be from something recoverable like one of the ode solvers failing for a given set of parameters

Copy link
Member Author

@WardBrian WardBrian Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pathfinder_impl catches std::domain_errors, which is what the ODE solvers throw (and is generally what the math library considers 'recoverable'), so the only exceptions which reach this will be unrecoverable ones from the model or issues in our algorithm

src/stan/services/pathfinder/single.hpp Show resolved Hide resolved
@WardBrian
Copy link
Member Author

std::out_of_range would be considered fatal by this and lead to the algorithms terminating. Is there something else you're looking for clarification on about them?

@SteveBronder
Copy link
Collaborator

out of range can make sense, but what about std::invalid_argument? Like quad_form_sym would throw that for symmetric matrices. Would we just fail on that error and end the program?

@WardBrian
Copy link
Member Author

Those have been considered unrecoverable (at least during initialization) for a while. invalid_argument is thrown for things like check_square, which won't change if you just 'try again'.

I think if there are usages in Math we want to be recoverable, we should change those to match the others

@SteveBronder
Copy link
Collaborator

Okay looking at this again I think this is fine to merge 👍

@WardBrian WardBrian merged commit 6697a24 into develop Feb 13, 2024
3 checks passed
@WardBrian WardBrian deleted the fix/3258-unconditional-exception-swallowing branch February 13, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove places where exceptions are unconditionally swallowed.
2 participants