Stop VecNormalize rewriting the observation space of the env it wraps - #2286
Open
DenisDrobyshev wants to merge 1 commit into
Open
DenisDrobyshev wants to merge 1 commit into
DenisDrobyshev wants to merge 1 commit into
Conversation
The Dict branch wrote the normalized image bounds into the space dict it was handed, which the wrapped VecEnv and the env underneath it own, so a model built on the venv afterwards saw float32 where the env declared uint8. The Box branch already rebinds instead of writing through.
Author
|
Adjacent suites on the same checkout, since the description said I would add the counts: |
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.
Summary
VecNormalizereplaces an image sub-space with the normalized bounds, so that a policy built on top of it does not take the normalized floats for an image (#1214). For aDictobservation it writes that replacement into the space dict itself:VecEnvWrapper.__init__does not copy the space, soself.observation_spaceis the wrappedVecEnv's space, which is in turn the underlying env's space. Building the wrapper therefore changes the environment it wraps:The
Boxbranch of the same constructor, a dozen lines below, rebindsself.observation_spaceinstead of writing through it, so the non-dict case never had this.A model built on the venv afterwards then picks a different feature extractor for the image — and it is built on the venv, not on the
VecNormalize, which does not even have to be kept:The original bounds are not recoverable from the wrapper either:
self.obs_spacesis assigned before the loop, but it aliases the same dict, so it reports the rewritten space as well.Changes
stable_baselines3/common/vec_env/vec_normalize.py— theDictbranch copies the space before editing it, which is what theBoxbranch already effectively does. One line.docs/misc/changelog.md— an entry under Bug Fixes.tests/test_vec_normalize.py—test_vec_normalize_keeps_the_wrapped_obs_spaceasserts the venv's space is unchanged, and that the wrapper's own image sub-space is still rewritten tofloat32.Verification
tests/test_vec_normalize.py: 18 passed.With the fix reverted and the test kept, it fails on the first assertion, reporting
imgasBox(-10.0, 10.0, (64, 64, 1), float32)where the environment declaredBox(0, 255, (64, 64, 1), uint8). The test detects the defect rather than passing regardless.ruff check,ruff format --checkandmypyare clean on both files.