Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix pyright errors in envs and spaces folders #3061

Closed
wants to merge 18 commits into from
Closed

Fix pyright errors in envs and spaces folders #3061

wants to merge 18 commits into from

Conversation

ldfrancis
Copy link

@ldfrancis ldfrancis commented Sep 4, 2022 •
edited
Loading

Description

This PR aims to fix the pyright type errors that currently go unnoticed since the check is disabled in pyproject.toml

The proposed method of approach to accomplish this is to start with the gym folder and all sub-folders within. Then proceed to the tests folder. Once these errors are fixed, we then enable the check for type errors for the entire codebase.

At the moment, type errors are fixed for the folders;
gym/envs and gym/spaces

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

pseudo-rnd-thoughts
pseudo-rnd-thoughts approved these changes Sep 4, 2022
View reviewed changes
Copy link
Contributor

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I just have two questions

gym/envs/toy_text/frozen_lake.py Show resolved Hide resolved
gym/envs/toy_text/frozen_lake.py Show resolved Hide resolved
@pseudo-rnd-thoughts
Copy link
Contributor

@ldfrancis Hey, sorry I have been super busy with other stuff. could you fix the merge conflicts. I believe that #3072 will fix the mujoco issues that we were discussing and I have 1 comment to address

pseudo-rnd-thoughts
pseudo-rnd-thoughts suggested changes Sep 15, 2022
View reviewed changes
gym/envs/box2d/car_racing.py Outdated Show resolved Hide resolved
gym/envs/box2d/lunar_lander.py Show resolved Hide resolved
pseudo-rnd-thoughts
pseudo-rnd-thoughts reviewed Sep 15, 2022
View reviewed changes
gym/envs/box2d/car_racing.py
assert self.car is not None
if action is not None:
if self.continuous:
assert isinstance(action, np.ndarray)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry on the discussion on np.clip I was talking about Car Racing not Lunar Lander, my mistake

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no call to np.clip in CarRacing. So passing a tensor would cause an error when the environment internals try to use the elements of the tensor. We can alleviate this by additionally calling action = np.array(action) which ensures tensor conversion to numpy arrays.
Hence, as it is, the environment would not work with tensors, even without the assert that ensures actions are numpy arrays.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove it for now

@pseudo-rnd-thoughts
Copy link
Contributor

Could you resolve all of the discussion which are closed

@ldfrancis
Update pyright fix in car_racing.py
959c948 
In 'step' method, discourage users from
passing in None as action, and type ignore
the use of None when calling 'step' in 'reset'
@RedTachyon
Copy link
Contributor

I'm slightly confused with how this PR works exactly. As far as I can tell, there are no changes to the pyproject.toml configuration, so there is no actual difference in which type checks now pass, correct? Since they weren't failing previously, and nothing new is enabled.

Given that, I'm strongly opposed to each of the # type: ignore annotations. They are band-aids which make sense in certain cases, but not as a blanket way to hide all errors/warnings - especially if we're not enabling any new type checks, and if there's so many of them.

There is a few other changes that I can see between the ignores, some of them are reasonable, some are potentially problematic (e.g. casting any np.int or a zero-dim np.ndarray to int in several places, I'd really rather not do that, and if pyright says it's not allowed, we should figure out why it's not allowed in the type system and fix that instead)

@jkterry1
Copy link
Collaborator

Closing in favor of Gymnasium https://github.com/Farama-Foundation/Gymnasium

@jkterry1 jkterry1 closed this Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pseudo-rnd-thoughts pseudo-rnd-thoughts pseudo-rnd-thoughts requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ldfrancis @pseudo-rnd-thoughts @RedTachyon @jkterry1