Raises exceptions from initialization callbacks inside SimContext #2166
Conversation
oahmednv
requested review from
kellyguo11,
jsmith-bdai,
pascal-roth,
Mayankm96 and
jtigue-bdai
as code owners
There was a problem hiding this comment.
Since all the callbacks fail/succeed after the simulation starts playing (i.e. after we do sim.reset() or sim.play(), I think we can get away with doing the checks at precisely those two locations. What do you think?
For example, we can move the check down to after we call super().reset inside sim.reset.
Also we can show the traceback of the error instead of just re-raising the exception as that can guide better towards where the error came from. You can use this function for that: https://docs.python.org/3/library/traceback.html#traceback.format_exc
Small note: We added callback inside manager base as well so we need to handle error there too 😬
| self._backend = sim.backend | ||
| self._device = sim.device | ||
| # initialize the asset | ||
| self._initialize_impl() |
There was a problem hiding this comment.
Maybe do a try catch only over this call instead of the whole function? Don't see the need to wrap around the whole thing.
| @@ -256,6 +256,9 @@ def __init__(self, cfg: SimulationCfg | None = None): | |||
| # you can reproduce the issue by commenting out this line and running the test `test_articulation.py`. | |||
| self._gravity_tensor = torch.tensor(self.cfg.gravity, dtype=torch.float32, device=self.cfg.device) | |||
|
|
|||
| # define a global variable to store the exceptions raised in the callback stack | |||
| builtins.ISAACLAB_CALLBACK_EXCEPTION = None | |||
There was a problem hiding this comment.
This should be a list incase multiple callbacks failed? For instance, when the contact sensor callback fails because asset creation failed, the current code will overwrite the exception from asset class. The user will only get the error of contact sensor failure which doesn't help them get to the true issue.
There was a problem hiding this comment.
We only raise the first exception that occurred. How would you raise a list of exceptions otherwise?
There was a problem hiding this comment.
Hm print all the errors through omni.log.error and raise the first one? But yeah you are right. Didn't see there was a "if none" check happening.
|
Also: We should make a ticket on Omniverse Kit side to exit threads more gracefully in standalone mode. What do you think? Can you take care of it please? |
I saw some of the tests are failing through the render check. We can potentially remove it from the render call yes.
The traceback is shown as well as the error
|
| # check if we need to raise an exception that was raised in a callback | ||
| if builtins.ISAACLAB_CALLBACK_EXCEPTION is not None: | ||
| exception_to_raise = builtins.ISAACLAB_CALLBACK_EXCEPTION | ||
| builtins.ISAACLAB_CALLBACK_EXCEPTION = None |
There was a problem hiding this comment.
No particular need to reset it to None since the program should exit?
Mayankm96
changed the title
Description
Handling exceptions when raised inside the initialization callbacks
Fixes #1025
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there