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

catch formulation errors and report feature and timestep #778

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

hellkite500
Copy link
Member

@hellkite500 hellkite500 commented Mar 27, 2024

Addressing #777, this PR adds a try/catch to intercept State_Exceptions thrown by formulations and adds the feature id and timestep the exception occurred at in the exception message.

Changes

  • Formulation calls to get_response are now wrapped with try/catch

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux
  • MacOS

@PhilMiller
Copy link
Contributor

This looks like it'll work fine, but wouldn't it make more sense inside get_response?

@hellkite500
Copy link
Member Author

hellkite500 commented Mar 27, 2024

wouldn't it make more sense inside get_response

I thought about that, but it would end up pretty heavily duplicated across the various BMI formulation implementations. Additionally, I have been looking at refactoring the connection of Formulations with Catchments, which would potentially remove the feature identity from the Formulation inheritance hierarchy. So I thought this would be a good place for now since this is essentially the time/feature entry point to the formulation and all the required information is available to report when something goes wrong.

@PhilMiller
Copy link
Contributor

Maybe this is the occasion to refactor those BMI formulation implementations to de-duplicate get_response, since none of its logic is or should be dependent on the underlying language of the BMI module.

@hellkite500
Copy link
Member Author

I have a working branch where I've started down that path (and ended up doing the config refactoring to clean up the Formulation manager to help). get_response is a bit of the legacy API carried around between the Formulation and the Catchment abstractions that I was aiming to declutter. I would put it in that scope and try to leave this as a simple QoL update for error reporting/management (as long as the functionality here carries into future refactoring efforts).

@hellkite500
Copy link
Member Author

Additionally, the context relevant to the formulation is unique as we expand the concept of attaching formulations to different "things".
e.g. cbc73a1
for a "DomainLayer" the feature context isn't relevant, but the layer context is. As we get tighter BMI/Formulation integration with things like routing, I think keeping this kind of error handling and context reporting will require working at higher levels of the stack.

@PhilMiller
Copy link
Contributor

Ok, that's fair for now.

@hellkite500 hellkite500 merged commit 16662c1 into NOAA-OWP:master Mar 27, 2024
19 checks passed
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.

2 participants