Enable mypy analysis for all files. #593

ianyfan · 2022-10-26T09:26:09Z

Description

EDIT: remaining issues have been resolved. The issues with env.horizon and evaluate_policy are to be taken upstream, and then depending on how quickly the turnaround is on those, either wait for them to be merged before merging this, or ignore the issues for now and un-ignore them once the fixes are done.

ORIGINAL:

Fixes #573 by adding the remaining type annotations. Some of the issues in mce_irl.py is resolved by changing env.horizon to int(env.horizon) which indicates that there would be a problem if env.horizon was not an integer (I think it can be np.inf?). Does this occur in practice (since this should necessitate a runtime check)?

One other issue in mce_irl.py I fixed by using a cast, which I guarded behind if TYPE_CHECKING:. I don't think there is a better way of doing this, since there is no other way of restricting the type to what we want.

Remaining issues:

src/imitation/algorithms/preference_comparisons.py:121: error: Argument 1 to "shuffle" of "Generator" has incompatible type "List[TrajectoryWithRew]"; expected "Union[_SupportsArray[dtype[Any]], _NestedSequence[_SupportsArray[dtype[Any]]], bool, int, float, complex, str, bytes, _NestedSequence[Union[bool, int, float, complex, str, bytes]]]"  [arg-type]
src/imitation/algorithms/preference_comparisons.py:644: error: Argument 1 to "choice" of "Generator" has incompatible type "List[TrajectoryWithRew]"; expected "Union[_SupportsArray[dtype[Any]], _NestedSequence[_SupportsArray[dtype[Any]]], bool, int, float, complex, str, bytes, _NestedSequence[Union[bool, int, float, complex, str, bytes]]]"  [arg-type]
tests/algorithms/test_bc.py:133: error: Item "None" of "Optional[Cache]" has no attribute "mkdir"  [union-attr]
tests/algorithms/test_bc.py:163: error: Item "None" of "Optional[Cache]" has no attribute "mkdir"  [union-attr]
tests/algorithms/test_bc.py:186: error: Argument 1 to "evaluate_policy" has incompatible type "ActorCriticPolicy"; expected "BaseAlgorithm"  [arg-type]
tests/algorithms/test_bc.py:196: error: Argument 1 to "evaluate_policy" has incompatible type "ActorCriticPolicy"; expected "BaseAlgorithm"  [arg-type]

The ones in preference_comparisons.py appear to be due to an over-conservative annotation in numpythough I may have missed something. If it is due to this, then I'm not sure if there is a better way to deal with the issue than to type: ignore it.

The issues in test_bc.py are also weird and may just be worth ignoring.
pytestconfig.cache is typed as Optional[Cache] so I'm not sure whether this implies that it could actually be None sometimes, or whether it is just typed that way. If the latter, either an extra assert is not None may be sufficient; if the former, some extra logic may be required to run the tests properly.

Testing

Ran mypy over relevant files, as well as ci/code_checks.sh.

codecov · 2022-10-26T16:12:47Z

Codecov Report

Merging #593 (a4b080f) into master (f64f5f8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a4b080f differs from pull request most recent head 0cd35d7. Consider uploading reports for the commit 0cd35d7 to get more accurate results

@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   97.46%   97.47%           
=======================================
  Files          85       85           
  Lines        8203     8233   +30     
=======================================
+ Hits         7995     8025   +30     
  Misses        208      208

Impacted Files	Coverage Δ
src/imitation/data/rollout.py	`100.00% <ø> (ø)`
src/imitation/algorithms/base.py	`98.78% <100.00%> (ø)`
src/imitation/algorithms/bc.py	`98.33% <100.00%> (ø)`
src/imitation/algorithms/dagger.py	`100.00% <100.00%> (ø)`
src/imitation/algorithms/density.py	`94.48% <100.00%> (ø)`
src/imitation/algorithms/mce_irl.py	`96.17% <100.00%> (+0.21%)`	⬆️
src/imitation/algorithms/preference_comparisons.py	`99.13% <100.00%> (ø)`
src/imitation/data/types.py	`98.20% <100.00%> (+0.01%)`	⬆️
src/imitation/rewards/serialize.py	`100.00% <100.00%> (ø)`
tests/algorithms/test_bc.py	`100.00% <100.00%> (ø)`
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

AdamGleave

Largely looks good, only some minor changes needed.

src/imitation/algorithms/preference_comparisons.py

src/imitation/algorithms/adversarial/common.py

src/imitation/algorithms/dagger.py

src/imitation/algorithms/density.py

AdamGleave · 2022-10-28T00:11:10Z

src/imitation/algorithms/mce_irl.py

@@ -45,7 +56,7 @@ def mce_partition_fh(
        \pi is a 3d array, indexed \pi[t,s,a].
    """
    # shorthand
-    horizon = env.horizon
+    horizon = int(env.horizon)


Can we add a TODO here so we remember to fix this before merge (if the plan is to fix it in a seals PR first), or open a GH issue to track it (if we want to merge this PR before fixing it upstream)?

I've opened a PR and added an extra check that should still be relevant even once the upstream has been changed, but can still add an extra comment if that would be helpful

AdamGleave · 2022-10-28T00:16:36Z

tests/algorithms/test_bc.py

@@ -183,7 +185,7 @@ def test_that_bc_improves_rewards(
 ):
    # GIVEN
    novice_rewards, _ = evaluation.evaluate_policy(
-        cartpole_bc_trainer.policy,
+        cartpole_bc_trainer.policy,  # type: ignore[arg-type]


Similar to with the seals upstream issue, let's add a TODO or GH issue for this.

src/imitation/algorithms/mce_irl.py

src/imitation/algorithms/preference_comparisons.py

tests/algorithms/test_bc.py

AdamGleave · 2022-10-28T00:30:26Z

On upstream: I don't think we're going to have time to turnaround the SB3 change before you wrap up. The seals one should be a short fix, and I can review and merge same-day, so let's push for fixing that one upstream though.

Co-authored-by: Daniel Pandori <[email protected]>

ianyfan · 2022-10-28T14:56:36Z

There's more places that are missing type annotations, but since they don't cause errors in mypy, I will leave them be, so as not to add any more noise to this PR.

AdamGleave

LGTM.

@Rocamonde, I made some commits to this PR, can you please review the diff: https://github.com/HumanCompatibleAI/imitation/pull/593/files/f3805f8c7e2af8dd4e3803325ae586dee9e6f7ef..dc76fb8323eb9458e2cd23dd53b72f1e6ed5d7fa

ianyfan · 2022-10-31T23:42:56Z

New changes look good to me, at least

Rocamonde · 2022-11-01T00:59:21Z

Apologies, this did not show up on my GitHub PR notifications. Will review tomorrow.

Rocamonde

Looks good, just one thing I noticed that might be a bug, do take a look.

src/imitation/algorithms/mce_irl.py

tests/algorithms/test_bc.py

tests/algorithms/test_mce_irl.py

Rocamonde · 2022-11-01T11:10:32Z

Yay! Thanks for doing this @ianyfan , nice to see what we started being nicely completed. This will certainly make our codebase more robust to random bugs and errors.

AdamGleave · 2022-11-02T00:58:04Z

Thanks for the review @Rocamonde, issues should be resolved now

Rocamonde

This is excellent work! LGTM. I left some comments with suggestions and questions, worth looking over them before merging to make sure we're happy with the direction of things.

setup.py

src/imitation/algorithms/dagger.py

Rocamonde · 2022-11-02T15:11:55Z

src/imitation/algorithms/density.py

@@ -343,7 +343,7 @@ def __call__(
        rew_array = np.asarray(rew_list, dtype="float32")
        return rew_array

-    def train_policy(self, n_timesteps: int = int(1e6), **kwargs):
+    def train_policy(self, n_timesteps: int = int(1e6), **kwargs: Any) -> None:


We could try to be more specific than this, but ParamSpec is not (yet) a very well supported feature by most type checkers, and it might not be worth the effort of typing it.

I've not used ParamSpec much so may be missing something, but how would it help us here? It seems designed in cases where the P = ParamSpec("P") variable is reused, like decorators or other higher-order functions. But if we're returning None I don't think it makes a difference.

I meant for Any, not for None haha. The problem with this is that any subtype has to take any kwarg in the signature, but you only really want it to take the kwargs that .learn() can take. But not sure how you can use ParamSpec for this now I think about it. I think it was designed as a generic that is filled in implicitly at the callsite, not for extracting the type signature of an explicit method (which tbh should be a valid use case).

Rocamonde · 2022-11-02T15:14:17Z

src/imitation/algorithms/mce_irl.py

@@ -182,7 +192,7 @@ def forward(
        self,
        observation: th.Tensor,
        deterministic: bool = False,
-    ):
+    ) -> NoReturn:


Interesting . Is this type not incompatible with the superclass? How does that work?

I think it works because NoReturn is the bottom type: i.e. a type that is a subtype of all other types. There's a good discussion at python/mypy#4116

Type theory is quite cool. I remember attending a talk in Cambridge recently on some people working on re-constructing the foundations of mathematics using a flavor of type theory.

src/imitation/algorithms/preference_comparisons.py

src/imitation/rewards/serialize.py

AdamGleave · 2022-11-04T05:24:21Z

@Rocamonde I believe I've addressed all outstanding comments on this, please do re-review when you have a moment. Note we'll need to merge this one before #603 because that removes the mypy excludes.

Rocamonde · 2022-11-07T14:34:59Z

Sorry about this. I reviewed the PRs sequentially from my inbox and only saw this should have been merged before now.

Rocamonde · 2022-11-07T15:00:55Z

Actually it was fine, the merge was trivial. Will wait for tests to run and then merge.

Rocamonde

LGTM

AdamGleave mentioned this pull request Oct 28, 2022

Resolve the remaining mypy issues #594

Closed

AdamGleave requested changes Oct 28, 2022

View reviewed changes

ianyfan and others added 6 commits October 28, 2022 14:37

[WIP] remaining mypy annotations

b5db98c

makedir -> mkdir in test_bc

a1aa210

Fix remaining issues

d247f73

Remove TYPE_CHECKING guard

708ebe1

Add extra checks for horizon type

57be740

Co-authored-by: Daniel Pandori <[email protected]>

Cleanup various annotations

b40046a

Co-authored-by: Daniel Pandori <[email protected]>

ianyfan force-pushed the ianyfan/typing branch from d713d93 to b40046a Compare October 28, 2022 13:48

ianyfan added 2 commits October 28, 2022 14:59

fix mistake

3e9ac54

Revert type annotation in density

f3805f8

ianyfan changed the title ~~[WIP] remaining mypy annotations~~ Enable mypy analysis for all files. Oct 28, 2022

ianyfan marked this pull request as ready for review October 28, 2022 14:55

AdamGleave added 4 commits October 28, 2022 14:13

Bump seals to version with int horizon and update type checks

12605f0

Revert some out-of-scope type changes

40d6d40

Check for infinite horizon value error

e990d74

Update TODO to point to GH issue

b89b7f0

AdamGleave approved these changes Oct 28, 2022

View reviewed changes

Check gradients are non-None

dc76fb8

Rocamonde suggested changes Nov 1, 2022

View reviewed changes

src/imitation/algorithms/mce_irl.py Outdated Show resolved Hide resolved

tests/algorithms/test_bc.py Outdated Show resolved Hide resolved

tests/algorithms/test_mce_irl.py Show resolved Hide resolved

AdamGleave added 2 commits November 1, 2022 17:57

Bugfix: add p.grad not p

758936a

Reflow comment

081a3cf

AdamGleave requested a review from Rocamonde November 2, 2022 00:57

Rocamonde reviewed Nov 2, 2022

View reviewed changes

AdamGleave added 6 commits November 3, 2022 21:18

Make seals dependency >= to allow new versions

b134fda

Merge remote-tracking branch 'origin/master' into ianyfan/typing

14ecd9c

Add overload for type

cf59783

Comment why we're checking for non-None grads

a376e93

Fix overloads

6416750

Remove duplicate TypeVar definition

42f3aec

AdamGleave requested a review from Rocamonde November 4, 2022 05:22

AdamGleave added 3 commits November 3, 2022 22:40

Fix lint

cb5371a

Fix type error introduced in upstream merge

26cd166

Pin pyglet to solve glPush bug

a4b080f

Merge remote-tracking branch 'origin/master' into ianyfan/typing

0cd35d7

Rocamonde approved these changes Nov 7, 2022

View reviewed changes

AdamGleave merged commit 23fae17 into master Nov 7, 2022

AdamGleave deleted the ianyfan/typing branch November 7, 2022 17:45

Enable mypy analysis for all files. #593

Enable mypy analysis for all files. #593

Uh oh!

Conversation

ianyfan commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Uh oh!

codecov bot commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AdamGleave left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AdamGleave commented Oct 28, 2022

Uh oh!

ianyfan commented Oct 28, 2022

Uh oh!

AdamGleave left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianyfan commented Oct 31, 2022

Uh oh!

Rocamonde commented Nov 1, 2022

Uh oh!

Rocamonde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rocamonde commented Nov 1, 2022

Uh oh!

AdamGleave commented Nov 2, 2022

Uh oh!

Rocamonde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AdamGleave commented Nov 4, 2022

Uh oh!

Rocamonde commented Nov 7, 2022

Uh oh!

Rocamonde commented Nov 7, 2022

ianyfan commented Oct 26, 2022 •

edited

Loading

codecov bot commented Oct 26, 2022 •

edited

Loading

AdamGleave left a comment •

edited

Loading