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.

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

Added type hints in utils package #964

Merged
merged 14 commits into from
May 13, 2023

Conversation

LetteraUnica
Copy link
Contributor

@LetteraUnica LetteraUnica commented May 5, 2023

Description

Type hinted all python modules under pettingzoo/utils. Pyright pre-commit checks were disabled on specific lines of code because they gave an error. More details in screenshots section.

When adding type hints I spotted some bugs, so I also edited some lines of code.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Screenshots

image
Error present even if we check attribute presence beforehand.

image
image
Same error but different way to check attribute presence. There are a few more errors like these, I won't show all of them here.

image
It says that action could be None, even if we perform a check at line 31.

image
In this case space is an instance of gymnasium.spaces.Space that does not have .low and .high attributes. However, we check in __init__ that the action spaces are Box spaces which in turn have the .low and .high attributes.

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • 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

@elliottower
Copy link
Contributor

Finally got a chance to look this over a bit, thanks for helping out with this. I see there are a few TypeErrors to fix (https://github.com/Farama-Foundation/PettingZoo/actions/runs/4897571285/jobs/8745765176?pr=964):

 ERROR test/all_parameter_combs_test.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
ERROR test/doc_examples_test.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
ERROR test/pickle_test.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
ERROR test/pytest_runner_test.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
ERROR test/specific_env_test.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
ERROR test/unwrapped_test.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
ERROR test/variable_env_test.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
!!!!!!!!!!!!!!!!!!! Interrupted: 7 errors during collection !!!!!!!!!!!!!!!!!!!!

@LetteraUnica
Copy link
Contributor Author

Yeah I saw them, however they are only present in Python version <3.10, because all the tests for python 3.10 pass. Do you think I should substitute the | operator with Union?
For example, a: int | None -> a: Union[int, None]

@elliottower
Copy link
Contributor

elliottower commented May 11, 2023 via email

@elliottower elliottower marked this pull request as ready for review May 11, 2023 19:49
@LetteraUnica
Copy link
Contributor Author

I imported annotations in all files apart from clip_out_of_bounds_wrapper which caused the error. Pushed the changes and now it all seems ok.

I forgot to mention that I haven't added type hints to utils/conversions as I am not familiar with that part of the library and the code seems very complicated.

@elliottower elliottower added enhancement New feature or request in progress labels May 13, 2023
@elliottower
Copy link
Contributor

Looks like all the comments are resolved, I’ll rise one final look through today but probably good to go. Would you mind making a separate PR changing the self state to be ndarray or any? Or I guess just any. I think I’ve seen something or any before as a way of suggesting but not enforcing

@LetteraUnica
Copy link
Contributor Author

LetteraUnica commented May 13, 2023

Just pushed the last changes, I think we can close this PR as well.

If I understand correctly, in subsequent PRs what remains to do is:

  • Make env.state return Any.
  • Where possible add assertions instead of ignoring pyright type errors.

Should I open an issue as a reminder? Because I don't think I'll have time to do this till next weekend.

Copy link
Contributor

@elliottower elliottower left a comment

Choose a reason for hiding this comment

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

A few minor nitpicks but overall very good, thanks again for this it is very nice to have.

@elliottower elliottower merged commit 5c75ca6 into Farama-Foundation:master May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants