Type hints #293
Type hints #293
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| envs = gym.vector.SyncVectorEnv( | ||
| [make_env(args.env_id, args.seed + i, i, args.capture_video, run_name) for i in range(args.num_envs)] | ||
| ) | ||
| ) # type:ignore[abstract] |
There was a problem hiding this comment.
SyncVectorEnv inherits from VectorEnv which inherits from Env. For older gym versions (I'm currently on 0.23.1), Env is an ABC with abstract method render that is not overriden by any of the vector envs. Since it's fixed in the newest gym release and not our issue, I'm ignoring here.
| # Handling gym shapes being Optionals (variant 1) | ||
| # Personally i'd prefer the asserts | ||
| assert isinstance(envs.single_observation_space.shape, tuple), "shape of observation space must be defined" | ||
| assert isinstance(envs.single_action_space.shape, tuple), "shape of action space must be defined" | ||
|
|
||
| # Handling gym shapes being Optionals (variant 2) | ||
| # Once could also cast inside each call but in my eyes that's not conducive to readability | ||
| obs_space_shape = cast(tuple[int, ...], envs.single_observation_space.shape) | ||
| action_space_shape = cast(tuple[int, ...], envs.single_action_space.shape) |
There was a problem hiding this comment.
Gym spaces can in theory return None shapes. Mypy will complain about this when concatenating the shape tuples later.
- Option 1 is to use a cast either once here or every time the spaces are accessed. I don't think that's very readable.
- Option 2 is to assert that the space shapes are tuples. Doing it once here fixes all errors for the rest of the code. Since there's an assert in this place already anyway I think this is the better option.
| ) | ||
|
|
||
| def forward(self, x): | ||
| def forward(self, x: torch.FloatTensor) -> torch.FloatTensor: |
There was a problem hiding this comment.
I've used FloatTensor here but we should just use torch.Tensor if type hints are pursued further.
vwxyzjn
left a comment
vwxyzjn
left a comment
There was a problem hiding this comment.
Thank you! The PR looks good. I have left some comments
| # ALGO LOGIC: initialize agent here: | ||
| class QNetwork(nn.Module): | ||
| def __init__(self, env): | ||
|
|
| ) | ||
|
|
||
| def forward(self, x): | ||
| def forward(self, x: torch.FloatTensor) -> torch.FloatTensor: |
| # Handling gym shapes being Optionals (variant 1) | ||
| # Personally i'd prefer the asserts | ||
| assert isinstance(envs.single_observation_space.shape, tuple), "shape of observation space must be defined" | ||
| assert isinstance(envs.single_action_space.shape, tuple), "shape of action space must be defined" | ||
|
|
||
| # Handling gym shapes being Optionals (variant 2) | ||
| # Once could also cast inside each call but in my eyes that's not conducive to readability | ||
| obs_space_shape = cast(tuple[int, ...], envs.single_observation_space.shape) | ||
| action_space_shape = cast(tuple[int, ...], envs.single_action_space.shape) |
| def get_action_and_value(self, x, action=None): | ||
| def get_action_and_value( | ||
| self, x: torch.Tensor, action: Optional[torch.Tensor] = None | ||
| ) -> tuple[torch.Tensor, torch.Tensor, torch.Tensor, torch.Tensor]: |
There was a problem hiding this comment.
Adding from __future__ import annotations at the top would guarantee the 3.7+ compatibility, and make dropping support later on quite easy.
3 participants
Description
As discussed on Discord, I've done basic type hints for PPO and DQN. Everything checks out with mypy 0.982 (
mypy cleanrl/ppo.py --show-error-codes --ignore-missing-imports). Of course we can have a discussion about whether that's the checker that will be used if we go further down the road of implementing this. I'll put comments in noteworthy places.The tests fail because
tuple[int]orlist[int]only works from Python 3.9 on.Types of changes
Checklist:
pre-commit run --all-filespasses (required).