Refactoring/mypy issues test

[dantp-ai]

Improves typing in examples and tests, towards mypy passing there.

Introduces the SpaceInfo utility

[codecov-commenter]

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (6e1ffe5) 88.24% compared to head (89bb655) 88.24%.
Report is 6 commits behind head on master.

Files	Patch %	Lines
tianshou/utils/space_info.py	88.52%	7 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1017   +/-   ##
=======================================
  Coverage   88.24%   88.24%           
=======================================
  Files          98       99    +1     
  Lines        8083     8144   +61     
=======================================
+ Hits         7133     7187   +54     
- Misses        950      957    +7     

Flag	Coverage Δ
unittests	88.24% <88.52%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MischaPanch]

Is this ready for review? If yes, then pls include the tests directory into the poe task (inside pyproject.toml) and make sure CI is green

[dantp-ai]

Noted.

Re: status, not ready for review yet. I think I can finish resolving the issues in the test by the end of this weekend.

[MischaPanch]

Great, I'm marking it as draft until then. Mark it as ready and assign me once you're done please

[MischaPanch]

There's been some changes on master now. They likely won't affect the tests (they do affect examples though) but it would still be better to merge master into your branch before continuing.

[dantp-ai]

Issue 1:

Fixing [attr-defined] for different types of spaces. Here is one example:

    env = gym.make(args.task)
    args.state_shape = env.observation_space.shape or env.observation_space.n
    args.action_shape = env.action_space.shape or env.action_space.n
    args.max_action = env.action_space.high[0]  # float

that gives this error:

test/offline/test_td3_bc.py:77: error: "Space[Any]" has no attribute "n"  [attr-defined]
        args.state_shape = env.observation_space.shape or env.observation_space.n
                                                          ^~~~~~~~~~~~~~~~~~~~~~~
test/offline/test_td3_bc.py:78: error: "Space[Any]" has no attribute "n"  [attr-defined]
        args.action_shape = env.action_space.shape or env.action_space.n
                                                      ^~~~~~~~~~~~~~~~~~
test/offline/test_td3_bc.py:79: error: "Space[Any]" has no attribute "high"  [attr-defined]
        args.max_action = env.action_space.high[0]  # float

Solution 1:

Check for the action_space/observation_space instance:

if isinstance(env.action_space, Box):
    args.action_shape = env.action_space.shape
    args.max_action = env.action_space.high[0]  # Access the 'high' attribute for the upper bound
    args.action_dim = args.action_shape[0]
elif isinstance(env.action_space, Discrete):
    args.action_shape = env.action_space.n
    args.max_action = env.action_space.start + env.action_space.n - 1
    args.action_dim = args.n
else:
    raise NotImplementedError("Action space is not of type `Box` or `Discrete`.")

I am only checking for Box and Discrete as an example, but if we choose this solution, we should also consider other spaces such as Graph, etc.

Solution 2:

Tell mypy to ignore the [attr-defined] error in these cases.

Outcome

I prefer Solution 1 or something similar to it. What do you think?

Issue 2:

The issue above repeats itself in different test files. It would be better to move the solution implementation in a utils or commons module to be used by all the tests instead of applying the same changes in dozens of files. What do you think?

[MischaPanch]

I also prefer solution 1, and feel free to write small util functions for extracting such info from an unknown space into the source code to reduce duplication.

I'm not supper happy that something called action_shape can hold an int, is there an easy way to not use the same var name for continuous and discrete cases?

[MischaPanch]

Let me know when you're done and the additional checks have been included in CI, then I'd have a final look and we can merge it
@dantp-ai

[dantp-ai]

Yep, for sure! I'll let you know once it's ready for review or some additional thing needs a closer look from your side.

I'm working on the updates using SpaceInfo at the moment.

[dantp-ai]

I'll have a big update coming today (will ping again here), that we should consider for review toward closing this PR.

We can solve any remaining mypy issues (not too many left) in a new PR.

[carlocagnetta]

I just opened the other PR running mypy on notebooks #1041. Checking your changes I don't think this should affect you, but it might rather resolve some more mypy errors.
Note possible conflict in the pyproject since I add a poe sub command to run mypy and ruff checks on notebooks.

[dantp-ai]

I pushed now the changes for review.

I didn't add examples and test dir to poe type-check because there are still some mypy issues open.

The PR is quite big already, and I thought we could maybe close this one now after the review. Then I can solve the other issues in a second PR.

[MischaPanch]

Thanks a lot for this work! Sorry for my delayed reaction, I was on vacation till today. Merging this now, and looking forward to the follow-up that will close the remaining mypy issues

Note: at many places there is something like policy: GailPolicy = GailPolicy(...). I don't think this is required, am I wrong?