Repository navigation
[Deprecation] Finalize v0.15 default set_state behavior and remove transitional logic - #4501
Conversation
…ansitional logic This commit completes the v0.15 transition for environment state initialization, moving the default behavior from implicitly honoring provided states (with warnings) to explicitly generating fresh states unless set_state=True is passed. Architectural Improvements: - Updated the default `set_state` argument in `EnvBase.reset` and `EnvBase.rollout` from `None` to `False` to statically define the intended behavior. - Removed the deprecated `FutureWarning` that manually honored implicit state. - Eradicated `select_reset_only` from the codebase. Since `reset()` now strictly ignores states by default, manually stripping state keys during automatic resets (e.g., in rollouts) is obsolete. - Cleaned up obsolete helper methods (`_input_td_has_state`) across `EnvBase`, `TransformedEnv`, and `IsaacLabEnv` that existed solely to support the removed warning branch. - Updated mock classes and regression tests to strictly enforce out-of-place tensordict modifications and assert the new externally observable behavior. This refactor reduces environment stepping overhead by removing redundant TensorDict selection layers and ensures a safer, more predictable state management contract across all TorchRL environments.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/4501
Note: Links to docs will display an error until the docs builds have been completed. ❌ 9 New Failures, 1 PendingAs of commit b36351b with merge base 06d57a0 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@torchrlbot reviewer @theap06 |
|
Requested review from @theap06 (requested by @coder-jayp). |
| @@ -4310,7 +4259,7 @@ def maybe_reset(self, tensordict: TensorDictBase) -> TensorDictBase: | |||
| """ | |||
| any_done = self.any_done(tensordict) | |||
| if any_done: | |||
| tensordict = self.reset(tensordict, select_reset_only=True) | |||
| tensordict = self.reset(tensordict) | |||
There was a problem hiding this comment.
the issue with no longer setting to select_reset_only is that dropping the reset_keys breaks the auto-reset for envs that don't have set_state. OpenSpielEnv, for example, does this as the state is deserialized from the td. I would say to keep the select in maybe_reset.
There was a problem hiding this comment.
I've restored the select_reset_only logic in EnvBase to keep auto-resets safe.
| @@ -2757,6 +2757,8 @@ def __init__(self, target: int = 5, batch_size: int | None = None): | |||
| def _reset(self, tensordict: TensorDict | None = None, **kwargs) -> TensorDict: | |||
| if tensordict is None: | |||
| tensordict = TensorDict(batch_size=self.batch_size, device=self.device) | |||
| else: | |||
There was a problem hiding this comment.
The issue right now is that the _reset envs now crash on auto-reset. An env whose _reset writes into the tensordict it receives and returns it now raises an error.
There was a problem hiding this comment.
I had initially patched the mock with .empty() to get around the strict out-of-place check after dropping select_reset_only. Since we're keeping the select logic, the mock is reverted back to its original state.
| @@ -3113,7 +3113,7 @@ def reset( | |||
| self, | |||
| tensordict: TensorDictBase | None = None, | |||
| *, | |||
| set_state: bool | None = None, | |||
| set_state: bool = False, | |||
There was a problem hiding this comment.
this breaks the chess tests,TestChessEnv resets without set_state=True so 36 of them fail now. those callers need migrating, and envs_api.rst and the pendulum tutorial still describe the old behavior
There was a problem hiding this comment.
I've migrated all the FEN reset callers in TestChessEnv to pass set_state=True, and updated both envs_api.rst and the pendulum tutorial to document set_state=False as the official v0.15 default.
… docs for set_state=False default
theap06
left a comment
There was a problem hiding this comment.
Thanks for working on this back-and-forth! the select_reset_only ValueError test got dropped but the check is still there, and the pr body still says select_reset_only was removed. Can you fix that? Also, asyncenvpool.reset drops the kwargs so it never reaches the sub-envs. also, can you raise NotImplementedError on the pool when set_state=True for now? we should draft a proper forwarding in a follow-up pr. lmk if you would like to take up the follow up pr. for the follow up pr, I would like to see an issue with the reprod example before drafting up the pr.
…ntedError in AsyncEnvPool
|
@theap06 Restored the ValueError test, updated the PR body, and added a NotImplementedError in EnvPoolBase.reset for set_state=True. I'll take up the follow-up PR. Will open an issue with a repro script for the pool kwarg-dropping shortly. |
This PR completes the v0.15 transition for environment state initialization, moving the default behavior from implicitly honoring provided states (with warnings) to explicitly generating fresh states unless set_state=True is passed.
Architectural Improvements:
set_stateargument inEnvBase.resetandEnvBase.rolloutfromNonetoFalseto statically define the intended behavior.FutureWarningthat manually honored implicit state._input_td_has_state) acrossEnvBase,TransformedEnv, andIsaacLabEnvthat existed solely to support the removed warning branch.This refactor ensures a safer, more predictable state management contract across all TorchRL environments.
Description
This PR fully executes the
TODO(v0.15)directive intorchrl/envs/common.py. By changing the default value ofset_stateinEnvBase.reset()andEnvBase.rollout()toFalse, environments now safely and cleanly ignore state keys during automatic resets by default.Additionally, this PR cleans up the obsolete code (
_input_td_has_state) that was left behind after removing the v0.14FutureWarningtransition branch, and updates the behavioral regression tests to assert the new expected behavior cleanly.Motivation and Context
This change is required to finalize the deprecation cycle started in v0.14. It solves the issue of implicit state-honoring during automatic resets, which previously required workarounds and warnings to manage.
Types of changes
What types of changes does your code introduce? Remove all that do not apply:
(Note: While intended, this technically counts as a breaking change since it finalizes a deprecation warning and alters default state initialization behavior).
Checklist
Go over all the following points, and put an
xin all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!