[Fix] Surface the real error when environment construction fails, instead of an AttributeError from __del__ - #7373
Draft
hujc7 wants to merge 3 commits into
Draft
Conversation
The environment destructors read `_is_closed` unconditionally. A subclass may run fallible setup before delegating to the base `__init__` that assigns the flag: `ManagerBasedRLEnv` allocates `episode_length_buf` and `reset_buf` before calling `super().__init__()`, so an allocation failure there leaves the attribute unset. `__del__` then raises `AttributeError`, which Python reports as an ignored exception in place of the original construction error, making the real failure materially harder to diagnose. Default the lookup to `True` so an environment that never finished initializing is treated as already closed and needs no teardown. The buffer allocation cannot simply move after `super().__init__()`: manager construction calls every observation term once to measure its output shape, and some terms read `episode_length_buf`.
The inline comment restated rationale already carried by the previous commit message; keep only the fact that forces the `getattr`. The changelog claimed the destructor masked the original error, but the original exception still propagates to the caller: the spurious `AttributeError` is printed ahead of it, not in place of it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When environment construction fails partway through
__init__, the destructor emits a second,misleading traceback ahead of the real failure:
ManagerBasedEnv.__del__(and theDirectRLEnv/DirectMARLEnvequivalents) readself._is_closedunconditionally. That attribute is assigned by
ManagerBasedEnv.__init__, but a subclass may runfallible setup before delegating to it:
ManagerBasedRLEnv.__init__allocatesepisode_length_bufand
reset_bufoncfg.sim.devicebefore callingsuper().__init__(). A CUDA OOM, an invalid device,or a malformed config raises there, leaving
_is_closedunset.The original exception still propagates to the caller — it is not swallowed. But Python cannot raise
out of
__del__, so it routes theAttributeErrortosys.unraisablehook, which prints it onunbuffered stderr. The spurious traceback therefore lands first, naming a different exception type
at a different location, which is what makes the real failure hard to find.
This defaults the lookup to
True, so an environment that never finished initializing is treated asalready closed and needs no teardown. That matches the existing contract:
_is_closed = Trueisalready the first statement of
ManagerBasedEnv.__init__for exactly this reason, and genuinepartial-init cleanup is handled explicitly by the
try/exceptaround_init_sim()that releases theSimulationContextsingleton. Applied to all three env classes, which share an identical destructor.Why not move the allocation after
super().__init__()? Manager construction calls everyobservation term once to measure its output shape (
ManagerBase.__init__->_prepare_terms->term_cfg.func(env)), and terms such asmdp.observations.current_time_sreadenv.episode_length_buf. The early allocation was introduced deliberately in #2332 for that reason.Guarding the destructor is the change that does not perturb construction order.
Fixes NVBugs 6604738. Related: 6591313 (
AssetBase.__del__) and 6311065 (direct_rl_env.__del__) —both fixed, neither covers this path;
AssetBaseuses a different attribute (_is_initialized).Type of change
Release backport
developValidation
test_env_destructors.pygains one parametrized case covering a partially constructed env. Verified itfails before the fix and passes after:
3 failed—AttributeErroratmanager_based_env.py:24814 passed(11 existing + 3 new)uv run --extra test python -m pytest source/isaaclab/test/envs/test_env_destructors.py -qKnown gap
This guards
__del__only._is_closedis also read unguarded inclose()atmanager_based_env.py:605,direct_rl_env.py:591,direct_marl_env.py:593,manager_based_rl_env.py:358andleapp_deployment_env.py:427, sotry: env = ManagerBasedRLEnv(cfg) finally: env.close()still raises. A follow-up declaring_is_closed: bool = Trueas a class attribute would cover every read site at once.Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdor my name already exists there