NOAA-OWP / ngen

Next Generation Water Modeling Engine and Framework Prototype
Other
82 stars 57 forks source link

re-throw exceptions as std::runtime_error with original error #801

Closed hellkite500 closed 1 month ago

hellkite500 commented 2 months ago

When getting time units from the base BMI class, exception context is lost from the subclasses which raise them. For example, delegating to to the python adapter where the python module raises an exception, the pybind::error_already_set exception and message is lost, and we are left with just an uncaught std::exception with no context. Catching the generic exception and copying the message into a runtime_error at least provides the error context provided by the original exception.

Changes

Checklist

Target Environment support

PhilMiller commented 2 months ago

Were we actually getting exceptions thrown from GetTimeUnits? Were they actually related to that, or something coming from a first call to a BMI module post-Initialize?

hellkite500 commented 2 months ago

Were we actually getting exceptions thrown from GetTimeUnits? Were they actually related to that, or something coming from a first call to a BMI module post-Initialize?

If a python module, for example, implemented get_time_units as

    def get_time_units(self) -> str:
        raise NotImplementedError

Then this function would error with just

libc++abi: terminating due to uncaught exception of type std::exception: std::exception

I did a quick test on other BMI functionality used via ngen and didn't see the same uncaught exception message. With the proposed changes, the above sitaution produces

libc++abi: terminating due to uncaught exception of type std::runtime_error: NotImplementedError: <EMPTY MESSAGE>

At:
  <path/to/module.py>(<line>): get_time_units
PhilMiller commented 2 months ago

That change in error message is definitely a big improvement.

In the BMI C adapter, we check every return, and throw on failures. Should each language BMI adapter be responsible for doing similarly? I.e. should Bmi_Py_Adapter catch whatever potentially gets thrown from any method, and wrap it up in a type that our higher-level language-independent code will recognize?