Update the type hinting for core.py #39
Conversation
| @@ -340,18 +338,18 @@ def __repr__(self): | |||
| return str(self) | |||
|
|
|||
| @property | |||
| def unwrapped(self) -> Env: | |||
| def unwrapped(self) -> Env[ObsType, ActType]: | |||
There was a problem hiding this comment.
Not necessarily true - consider the wrappers that discretize actions, or add pixel observations
There was a problem hiding this comment.
Good point, removed
There was a problem hiding this comment.
Added a bunch of extra comments.
And as a general note, please do keep the PRs a bit more atomic, or better described. A PR that only updates the type hints (as per the PR title) should have no reason to touch the tests whatsoever. If this were two separate PRs, one with types, and another with everything that affects tests, then we'd have one part that's safe to quickly review and merge, and then another part that needs careful review. Now, everything needs careful review.
gymnasium/core.py
Outdated
|
|
||
| ObsType = TypeVar("ObsType") | ||
| ActType = TypeVar("ActType") | ||
| RenderFrame = TypeVar("RenderFrame") | ||
|
|
||
|
|
||
| class Env(Generic[ObsType, ActType]): | ||
| class Env(Generic[ObsType, ActType], ABC): |
There was a problem hiding this comment.
So this is something I've been contemplating for some time, whether Env should be an ABC, and I was always leaning more towards "No". It kind of is, and kind of isn't. Notice that, for example, Env.reset isn't in fact an abstract method because it does have some logic in it. In general, ABCs are meant to be empty skeletons, but we have a good chunk of built-in implementations. So I'm still kinda on the fence, and crucially I'm not 100% sure what runtime implications this has exactly.
There was a problem hiding this comment.
Agreed with most of what you say, I thought this was a feature that I removed when re-adding python 3.6 support openai/gym#2836 so was re-add however that is false.
I think we either make Env, Env.reset and Env.step all abstract or none of them. This is primarily due to Env.step being a true abstract method unlike Env.reset. This makes me think that we don't need this change and should revert.
Do you agree?
There was a problem hiding this comment.
Yep, I'd keep it non-abstract altogether. I'd also strongly consider moving towards asking env developers to call super().step(action) and similarly for all the other methods, but this would be a relatively complex procedure that I don't have sufficient justification and motivation to push for at the moment.
There was a problem hiding this comment.
That makes sense and agree that super().step(action) could work but there isn't much good reasons for it other than we do it for reset
|
@RedTachyon Removing the I have kept the changes to The only major changes to before is two new TypeVars |
| """Returns an attribute with ``name``, unless ``name`` starts with an underscore.""" | ||
| if name.startswith("_"): | ||
| if name == "_np_random": |
There was a problem hiding this comment.
I'm not entirely sure about this, it's out of scope, and seems like a guardrail that's not necessarily needed.
There was a problem hiding this comment.
This was a guardrail that already existed, but it was bugged so didn't work. I had to modify this function to re-add it such that the correct error occurred.
We could replace it with a warning? I added it to gym a couple of months ago as I thought I had found a bug, but what was accurately happening was that I was using the wrapper _np_random not the environment _np_random which took me about an hour + to solve. Therefore, I proposed this solution. I guess the issue is that wrappers could modify the reproducibility of an environment using the environment rng not the wrapper rng however in this case, the wrapper could use its own rng with a different variable name. Thoughts?
There was a problem hiding this comment.
I think it's fine for now, we can reconsider this guardrail in general in the future
tests/test_core.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize("class_", [UnittestEnv, UnknownSpacesEnv]) | ||
| @pytest.mark.parametrize("props", properties) | ||
| def test_wrapper_property_forwarding(class_, props): |
There was a problem hiding this comment.
I'm still a bit hesitant about removing the existing tests.
Are these tests redundant now? Do they fail with the new changes? (if so, that's a potential problem) Are they 100% replaced with the new tests?
I'd lean towards keeping them in, and just adding new tests instead. And if some tests turn out to be unnecessary, we can remove them separately with no other changes.
There was a problem hiding this comment.
Ok, I will keep both then and we can decide what tests to keep later then
# Conflicts: # gymnasium/core.py
# Conflicts: # gymnasium/core.py
| """Returns an attribute with ``name``, unless ``name`` starts with an underscore.""" | ||
| if name.startswith("_"): | ||
| if name == "_np_random": |
There was a problem hiding this comment.
I think it's fine for now, we can reconsider this guardrail in general in the future
* Fix failing set_env test * Fix test failiing due to deprectation of env.seed * Adjust mean reward threshold in failing test * Fix her test failing due to rng * Change seed and revert reward threshold to 90 * Pin gym version * Make VecEnv compatible with gym seeding change * Revert change to VecEnv reset signature * Change subprocenv seed cmd to call reset instead * Fix type check * Add backward compat * Add `compat_gym_seed` helper * Add goal env checks in env_checker * Add docs on HER requirements for envs * Capture user warning in test with inverted box space * Update ale-py version * Fix randint * Allow noop_max to be zero * Update changelog * Update docker image * Update doc conda env and dockerfile * Custom envs should not have any warnings * Fix test for numpy >= 1.21 * Add check for vectorized compute reward * Bump to gym 0.24 * Fix gym default step docstring * Test downgrading gym * Revert "Test downgrading gym" This reverts commit 0072b77. * Fix protobuf error * Fix in dependencies * Fix protobuf dep * Use newest version of cartpole * Update gym * Fix warning * Loosen required scipy version * Scipy no longer needed * Try gym 0.25 * Silence warnings from gym * Filter warnings during tests * Update doc * Update requirements * Add gym 26 compat in vec env * Fixes in envs and tests for gym 0.26+ * Enforce gym 0.26 api * format * Fix formatting * Fix dependencies * Fix syntax * Cleanup doc and warnings * Faster tests * Higher budget for HER perf test (revert prev change) * Fixes and update doc * Fix doc build * Fix breaking change * Fixes for rendering * Rename variables in monitor * update render method for gym 0.26 API backwards compatible (mode argument is allowed) while using the gym 0.26 API (render mode is determined at environment creation) * update tests and docs to new gym render API * undo removal of render modes metatadata check * set rgb_array as default render mode for gym.make * undo changes & raise warning if not 'rgb_array' * Fix type check * Remove recursion and fix type checking * Remove hacks for protobuf and gym 0.24 * Fix type annotations * reuse existing render_mode attribute * return tiled images for 'human' render mode * Allow to use opencv for human render, fix typos * Add warning when using non-zero start with Discrete (fixes #1197) * Fix type checking * Bug fixes and handle more cases * Throw proper warnings * Update test * Fix new metadata name * Ignore numpy warnings * Fixes in vec recorder * Global ignore * Filter local warning too * Monkey patch not needed for gym 26 * Add doc of VecEnv vs Gym API * Add render test * Fix return type * Update VecEnv vs Gym API doc * Fix for custom render mode * Fix return type * Fix type checking * check test env test_buffer * skip render check * check env test_dict_env * test_env test_gae * check envs in remaining tests * Update tests * Add warning for Discrete action space with non-zero (#1295) * Fix atari annotation * ignore get_action_meanings [attr-defined] * Fix mypy issues * Add patch for gym/gymnasium transition * Switch to gymnasium * Rely on signature instead of version * More patches * Type ignore because of Farama-Foundation/Gymnasium#39 * Fix doc build * Fix pytype errors * Fix atari requirement * Update env checker due to change in dtype for Discrete * Fix type hint * Convert spaces for saved models * Ignore pytype * Remove gitlab CI * Disable pytype for convert space * Fix undefined info * Fix undefined info * Upgrade shimmy * Fix wrappers type annotation (need PR from Gymnasium) * Fix gymnasium dependency * Fix dependency declaration * Cap pygame version for python 3.7 * Point to master branch (v0.28.0) * Fix: use main not master branch * Rename done to terminated * Fix pygame dependency for python 3.7 * Rename gym to gymnasium * Update Gymnasium * Fix test * Fix tests * Forks don't have access to private variables * Fix linter warnings * Update read the doc env * Fix env checker for GoalEnv * Fix import * Update env checker (more info) and fix dtype * Use micromamab for Docker * Update dependencies * Clarify VecEnv doc * Fix Gymnasium version * Copy file only after mamba install * [ci skip] Update docker doc * Polish code * Reformat * Remove deprecated features * Ignore warning * Update doc * Update examples and changelog * Fix type annotation bundle (SAC, TD3, A2C, PPO, base class) (#1436) * Fix SAC type hints, improve DQN ones * Fix A2C and TD3 type hints * Fix PPO type hints * Fix on-policy type hints * Fix base class type annotation, do not use defaults * Update version * Disable mypy for python 3.7 * Rename Gym26StepReturn * Update continuous critic type annotation * Fix pytype complain --------- Co-authored-by: Carlos Luis <[email protected]> Co-authored-by: Quentin Gallouédec <[email protected]> Co-authored-by: Thomas Lips <[email protected]> Co-authored-by: tlips <[email protected]> Co-authored-by: tlpss <[email protected]> Co-authored-by: Quentin GALLOUÉDEC <[email protected]>
* Fix failing set_env test * Fix test failiing due to deprectation of env.seed * Adjust mean reward threshold in failing test * Fix her test failing due to rng * Change seed and revert reward threshold to 90 * Pin gym version * Make VecEnv compatible with gym seeding change * Revert change to VecEnv reset signature * Change subprocenv seed cmd to call reset instead * Fix type check * Add backward compat * Add `compat_gym_seed` helper * Add goal env checks in env_checker * Add docs on HER requirements for envs * Capture user warning in test with inverted box space * Update ale-py version * Fix randint * Allow noop_max to be zero * Update changelog * Update docker image * Update doc conda env and dockerfile * Custom envs should not have any warnings * Fix test for numpy >= 1.21 * Add check for vectorized compute reward * Bump to gym 0.24 * Fix gym default step docstring * Test downgrading gym * Revert "Test downgrading gym" This reverts commit 0072b77156c006ada8a1d6e26ce347ed85a83eeb. * Fix protobuf error * Fix in dependencies * Fix protobuf dep * Use newest version of cartpole * Update gym * Fix warning * Loosen required scipy version * Scipy no longer needed * Try gym 0.25 * Silence warnings from gym * Filter warnings during tests * Update doc * Update requirements * Add gym 26 compat in vec env * Fixes in envs and tests for gym 0.26+ * Enforce gym 0.26 api * format * Fix formatting * Fix dependencies * Fix syntax * Cleanup doc and warnings * Faster tests * Higher budget for HER perf test (revert prev change) * Fixes and update doc * Fix doc build * Fix breaking change * Fixes for rendering * Rename variables in monitor * update render method for gym 0.26 API backwards compatible (mode argument is allowed) while using the gym 0.26 API (render mode is determined at environment creation) * update tests and docs to new gym render API * undo removal of render modes metatadata check * set rgb_array as default render mode for gym.make * undo changes & raise warning if not 'rgb_array' * Fix type check * Remove recursion and fix type checking * Remove hacks for protobuf and gym 0.24 * Fix type annotations * reuse existing render_mode attribute * return tiled images for 'human' render mode * Allow to use opencv for human render, fix typos * Add warning when using non-zero start with Discrete (fixes #1197) * Fix type checking * Bug fixes and handle more cases * Throw proper warnings * Update test * Fix new metadata name * Ignore numpy warnings * Fixes in vec recorder * Global ignore * Filter local warning too * Monkey patch not needed for gym 26 * Add doc of VecEnv vs Gym API * Add render test * Fix return type * Update VecEnv vs Gym API doc * Fix for custom render mode * Fix return type * Fix type checking * check test env test_buffer * skip render check * check env test_dict_env * test_env test_gae * check envs in remaining tests * Update tests * Add warning for Discrete action space with non-zero (#1295) * Fix atari annotation * ignore get_action_meanings [attr-defined] * Fix mypy issues * Add patch for gym/gymnasium transition * Switch to gymnasium * Rely on signature instead of version * More patches * Type ignore because of Farama-Foundation/Gymnasium#39 * Fix doc build * Fix pytype errors * Fix atari requirement * Update env checker due to change in dtype for Discrete * Fix type hint * Convert spaces for saved models * Ignore pytype * Remove gitlab CI * Disable pytype for convert space * Fix undefined info * Fix undefined info * Upgrade shimmy * Fix wrappers type annotation (need PR from Gymnasium) * Fix gymnasium dependency * Fix dependency declaration * Cap pygame version for python 3.7 * Point to master branch (v0.28.0) * Fix: use main not master branch * Rename done to terminated * Fix pygame dependency for python 3.7 * Rename gym to gymnasium * Update Gymnasium * Fix test * Fix tests * Forks don't have access to private variables * Fix linter warnings * Update read the doc env * Fix env checker for GoalEnv * Fix import * Update env checker (more info) and fix dtype * Use micromamab for Docker * Update dependencies * Clarify VecEnv doc * Fix Gymnasium version * Copy file only after mamba install * [ci skip] Update docker doc * Polish code * Reformat * Remove deprecated features * Ignore warning * Update doc * Update examples and changelog * Fix type annotation bundle (SAC, TD3, A2C, PPO, base class) (#1436) * Fix SAC type hints, improve DQN ones * Fix A2C and TD3 type hints * Fix PPO type hints * Fix on-policy type hints * Fix base class type annotation, do not use defaults * Update version * Disable mypy for python 3.7 * Rename Gym26StepReturn * Update continuous critic type annotation * Fix pytype complain --------- Co-authored-by: Carlos Luis <[email protected]> Co-authored-by: Quentin Gallouédec <[email protected]> Co-authored-by: Thomas Lips <[email protected]> Co-authored-by: tlips <[email protected]> Co-authored-by: tlpss <[email protected]> Co-authored-by: Quentin GALLOUÉDEC <[email protected]>
As core.py is the foundation for the whole of Gymnasium, it should have good type hinting to help users understand the expected types for functions.
This PR fills the gaps in the implementation where type hints were currently missing.
One of the things not done in this PR is to make this require pyright strict type hinting to enforce a number of type hinting specifications that are not applied currently to the project
Edit: I have added
ABCandabstractmethodtoEnv,Wrapper,Env.step,Env.reset