Move Pyright to `pre-commit` + add `pydocstyle` #737

kir0ul · 2022-07-06T05:44:04Z

Description

This PR moves the Pyright checks from CI only to pre-commit (both local + CI), fixes some typing issues, and add also pydocstyle to pre-commit.

Type of change

Refactoring/maintenance

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.
New and existing unit tests pass locally with my changes

pyproject.toml

pettingzoo/utils/env.py

jjshoots · 2022-07-06T11:19:24Z

pettingzoo/utils/env.py


 ObsType = TypeVar("ObsType")
 ActionType = TypeVar("ActionType")
-AgentID = str
+AgentID = Optional[str]


I'm not too familiar with dealing with typings, but is there a reason why this is Optional and not just a str?

Optional means AgentID is also allowed to be None. I don't have a lot of experience with typing either but if I don't put Optional, I get the following error:

./PettingZoo/pettingzoo/utils/env.py:239:22 - error: Cannot assign member "agent_selection" for type "AECEnv" Expression of type "AgentID | None" cannot be assigned to member "agent_selection" of class "AECEnv" Type "AgentID | None" cannot be assigned to type "AgentID" Type "None" cannot be assigned to type "AgentID" (reportGeneralTypeIssues)

@RedTachyon could you perhaps provide some insight on this?

Making it optional defeats the point (unless there is a valid situation in AEC where self.agent_selection == None, which I doubt)

This is a result of slightly sloppy implementation together with slightly sloppy type inference. If you look at the code, you can see that self._skip_agent_selection can be None, so the type checker thinks that self.agent_selection can also be None. We're making sure it won't happen by doing a getattr check, but that's probably not being evaluated by the type checker because it would have to evaluate an arbitrary string.

The simple solution is probably just adding a type guard assert self._skip_agent_selection is not None. The right solution would be refactoring the class so that we don't have attributes which might exist, and might not exist.

pettingzoo/utils/env.py

.pre-commit-config.yaml

pettingzoo/utils/env.py

pyproject.toml

kir0ul · 2022-07-06T18:08:59Z

pettingzoo/utils/env.py


 ObsType = TypeVar("ObsType")
 ActionType = TypeVar("ActionType")
-AgentID = str
+AgentID = Optional[str]


Optional means AgentID is also allowed to be None. I don't have a lot of experience with typing either but if I don't put Optional, I get the following error:

./PettingZoo/pettingzoo/utils/env.py:239:22 - error: Cannot assign member "agent_selection" for type "AECEnv" Expression of type "AgentID | None" cannot be assigned to member "agent_selection" of class "AECEnv" Type "AgentID | None" cannot be assigned to type "AgentID" Type "None" cannot be assigned to type "AgentID" (reportGeneralTypeIssues)

pettingzoo/utils/env.py

jjshoots · 2022-07-24T05:02:37Z

Hey @kir0ul, just checking in, what're the future plans for this PR?

kir0ul · 2022-07-26T20:42:31Z

Hey @kir0ul, just checking in, what're the future plans for this PR?

Hey @jjshoots, sorry I haven't forgotten about this, I'm currently attending a 3 weeks summer school, so I'll be able to resume working on this in early August. I guess the plan would be to:

clean up the comments,
fix Move Pyright to pre-commit + add pydocstyle #737 (comment), do we want to do it the simple way or start a refactoring?
maybe include pydocstyle (I haven't made my mind yet about including it or not as there are a lot of docstrings to fix).

jjshoots · 2022-08-03T08:53:38Z

Hey @kir0ul, if it's not terribly difficult, I'd prefer the refactoring route, although a simple assert doesn't seem like the end of the world to me either.

jjshoots · 2022-08-15T16:01:38Z

Hey @kir0ul, this is done now?

jjshoots · 2022-08-15T16:02:36Z

Hey @RedTachyon, could you review this when you have the time?

kir0ul · 2022-08-16T09:50:57Z

Hey @kir0ul, this is done now?

Hey @jjshoots, there should still be a few points of discussion but I think it's ready for review.

pettingzoo/utils/env.py

RedTachyon · 2022-08-16T10:42:04Z

pyproject.toml

@@ -1,16 +1,15 @@
 [tool.pyright]

 # add any files/directories with type declaration to include
-include = ["pettingzoo/utils/env.py"]
+include = [
+    "pettingzoo/utils/env.py"


For now we only want type checking for that single file?

I think it'd be better to include everything by default, and add explicit includes for each directory/file/env/whatever, so that new code is also type checked

If this is too big of a job, I'm more than ok with merging this PR first, and then starting new PRs for each of the other folders.

Yea, my point is changing the structure of include/exclude.

When done this way, let's say someone develops a new feature for PZ, puts it in a new file or directory or anywhere really, and pushes changes there. None of these changes will be type checked in any way.

Since c07a637#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711 it'll type check any file in the pettingzoo folder excluding the list of folders that haven't been refactored with typing yet. I also added types to files in pettingzoo/utils/. Does this work better?

kir0ul · 2022-08-17T10:22:55Z

pettingzoo/utils/conversions.py

-            return self._was_done_step(action)
-        self._actions[self.agent_selection] = action
+            return self._was_done_step(action)  # type: ignore
+        self._actions[self.agent_selection] = action  # type: ignore


Not sure how to remove those 2 # type: ignore statements, would anyone have any suggestion?

So I just checked, _was_done_step actually returns None. So you could probably just call the function, and return on a new line below it to break out of the function.

@jjshoots I tried your suggestion and I still get the same typing error, but maybe I missed something? 🤔

Do you need to do -> None at the function definition?

Ok now there's only one # type: ignore statement remaining!

Optional[int] is just a synonym for Union[int, None], i.e. it can be either an int or None

Yes but kir0ul mentioned that it was sometimes int or None, since the typechecker is happy with it being None, a more semantically accurate type hint would be Optional[int], no?

I tried Optional[int] too but I get:

./pettingzoo/utils/conversions.py ./pettingzoo/utils/conversions.py:264:40 - error: Argument of type "int | None" cannot be assigned to parameter "action" of type "None" in function "_was_done_step" Type "int | None" cannot be assigned to type "None" Type cannot be assigned to type "None" (reportGeneralTypeIssues) ./pettingzoo/utils/conversions.py:265:9 - error: Argument of type "int | None" cannot be assigned to parameter "__v" of type "None" in function "__setitem__" Type "int | None" cannot be assigned to type "None" Type cannot be assigned to type "None" (reportGeneralTypeIssues)

Also sometimes the type of action is np.int32.

This seems like a really annoying case of previously not enforcing types. Is it possible for us to fix the np.int32 thing using a type cast?

Done in 55c69e6#diff-3ef3437e8b392d934e60182d2116f18ce516d3685c43a4b40c2a5f84c0dfe93fR267.

jjshoots · 2022-08-21T15:55:54Z

@kir0ul is this ready now?

kir0ul · 2022-08-23T15:27:02Z

@jjshoots @RedTachyon I think this is ready for another round of review.

jjshoots · 2022-08-29T15:04:36Z

@RedTachyon do you mind giving this the next pass?

jjshoots · 2022-09-05T11:44:32Z

Optionally, could you give this a review? @pseudo-rnd-thoughts

pseudo-rnd-thoughts · 2022-09-07T22:10:51Z

I know it is a pain, but could we split this PR into two as it is two separate things being done. It will be easier to review that way.

jjshoots · 2022-09-07T22:40:44Z

I personally don't think that's such a great idea, the time spent splitting it would be non-trivial, but if @kir0ul doesn't mind then I'm ok with it.

RedTachyon · 2022-09-08T12:46:27Z

I don't see anything weird/concerning in the changes since my last pass, so it should be alright

jjshoots · 2022-09-08T15:27:38Z

@RedTachyon SGTM then.

This reverts commit 5c73f53.

kir0ul · 2022-09-09T14:19:24Z

Conflicts have been fixed and after talking with @jjshoots we decided not to split this PR in two.

jjshoots · 2022-09-09T14:35:30Z

Thanks a ton for the help everyone!

jjshoots reviewed Jul 6, 2022

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

jjshoots reviewed Jul 6, 2022

View reviewed changes

pettingzoo/utils/env.py Show resolved Hide resolved

jjshoots reviewed Jul 6, 2022

View reviewed changes

pettingzoo/utils/env.py Show resolved Hide resolved

jjshoots reviewed Jul 6, 2022

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

kir0ul commented Jul 6, 2022

View reviewed changes

kir0ul changed the title ~~Move Pyright to pre-commit~~ [WIP] Move Pyright to pre-commit Jul 6, 2022

kir0ul force-pushed the pyright_in_pre_commit branch from 4aa8876 to 7b59d23 Compare August 1, 2022 20:33

kir0ul force-pushed the pyright_in_pre_commit branch from 7b59d23 to 5c73f53 Compare August 3, 2022 22:25

kir0ul force-pushed the pyright_in_pre_commit branch 2 times, most recently from 43ba4a0 to b1bbb53 Compare August 12, 2022 00:14

kir0ul changed the title ~~[WIP] Move Pyright to pre-commit~~ [WIP] Move Pyright to pre-commit + add pydocstyle Aug 12, 2022

kir0ul marked this pull request as ready for review August 12, 2022 11:11

kir0ul changed the title ~~[WIP] Move Pyright to pre-commit + add pydocstyle~~ Move Pyright to pre-commit + add pydocstyle Aug 12, 2022

RedTachyon reviewed Aug 16, 2022

View reviewed changes

pettingzoo/utils/env.py Outdated Show resolved Hide resolved

RedTachyon reviewed Aug 16, 2022

View reviewed changes

kir0ul commented Aug 17, 2022

View reviewed changes

kir0ul added 17 commits September 8, 2022 22:44

ci: move pyright to pre-commit

b2d57f4

fix: typing

68c6759

fix: typing syntax for Python 3.7

5242fc6

fix: typing syntax for Py3.7 (bis)

40d8696

fix: ignore gym + numpy typing

6b4534e

fix: AgentID not optional str

e9fe845

ci: add pydoctsyle to pre-commit

bfd9e44

fix: remove type ignore for gym & numpy

d165c18

ci: fix pydocstyle

32545f3

Revert "fix: remove type ignore for gym & numpy"

a479e74

This reverts commit 5c73f53.

test: fix simple_v2

fdda44b

ci: fix typing in pettingzoo/utils/

dce1744

ci: fix reportMissingImports

4c710b0

docs: remember to remove Optional type

182468b

ci: remove type ignore statement

bd2ee98

ci: add None type to action argument

a43256a

fix: action type

52c1908

kir0ul force-pushed the pyright_in_pre_commit branch from 55c69e6 to 52c1908 Compare September 9, 2022 03:03

jjshoots merged commit aa28b75 into Farama-Foundation:master Sep 9, 2022

jjshoots mentioned this pull request Oct 13, 2022

[Proposal] Fix pyright code checking #691

Closed

Move Pyright to pre-commit + add pydocstyle #737

Move Pyright to pre-commit + add pydocstyle #737

Conversation

kir0ul commented Jul 6, 2022 • edited

Description

Type of change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedTachyon Jul 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjshoots commented Jul 24, 2022

kir0ul commented Jul 26, 2022

jjshoots commented Aug 3, 2022

jjshoots commented Aug 15, 2022

jjshoots commented Aug 15, 2022

kir0ul commented Aug 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kir0ul Aug 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjshoots Aug 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjshoots Aug 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjshoots commented Aug 21, 2022

kir0ul commented Aug 23, 2022

jjshoots commented Aug 29, 2022

jjshoots commented Sep 5, 2022

pseudo-rnd-thoughts commented Sep 7, 2022

jjshoots commented Sep 7, 2022

RedTachyon commented Sep 8, 2022

jjshoots commented Sep 8, 2022

kir0ul commented Sep 9, 2022

jjshoots commented Sep 9, 2022

Move Pyright to `pre-commit` + add `pydocstyle` #737

Move Pyright to `pre-commit` + add `pydocstyle` #737

kir0ul commented Jul 6, 2022 •

edited

RedTachyon Jul 7, 2022 •

edited

kir0ul commented Aug 16, 2022 •

edited

kir0ul Aug 17, 2022 •

edited

jjshoots Aug 17, 2022 •

edited

jjshoots Aug 23, 2022 •

edited