Fix mypy issues in tests and examples #1077
Conversation
* Creating a new instance of LoggerFactoryDefault within each module
* Remove func `compute_reward_fn` as it was defined already before
* Same variable name used multiple times for objects of different types
* index can be int or np.ndarray. since it's defined twice with the same name, mypy gets confused
* Make mypy happy and annotate some vars
* Use ActionSpaceInfo to type action space attrs * Print collector stats using DataclassPPrintMixin * Add missing type annotations to funcs
* Change default num epochs 1000 -> 25
…ic attrs and methods
… APIs using it. * `hidden_size` is replaced with `output_dim_added_layer` to better reflect its purpose, which is to add an output layer to DQN body net or not (based on whether `features_only`)
* it operates on Nets not Modules
The generic was needed because of the type of "state", representing the hidden state The only place where it is ever used is in recurrent version of DQN
…mples # Conflicts: # CHANGELOG.md
| obs_space_dtype = np.integer | ||
| elif np.issubdtype(type(obs_space.dtype), np.floating): | ||
| obs_space_dtype = np.floating | ||
| assert isinstance(obs_space, gym.spaces.Box) |
There was a problem hiding this comment.
Hi @dantp-ai ! Could you let me know what was the reason for this change? It leads to a broken DQN example on Atari for me due to obs_space_dtype being undefined. The type check only works if I check np.issubdtype(obs_space.dtype, np.integer), otherwise it will be False...
There was a problem hiding this comment.
Let me take a look how we can write this better.
The reason for the modification was a type mismatch previously between what Box allows for dtype compared to the more general type of Space for dtype:
examples/atari/atari_wrapper.py: note: In member "__init__" of class "WarpFrame":
examples/atari/atari_wrapper.py:212: error: Argument "dtype" to "Box" has incompatible type
"dtype[Any] | dtype[void] | None"; expected "type[floating[Any]] | type[integer[Any]]" [arg-type]
dtype=env.observation_space.dtype,
^~~~~~~~~~~~~~~~~~~~~~~~~~~
There was a problem hiding this comment.
... and also a numpy DeprecationWarning for setting np.integer as dtype.
There was a problem hiding this comment.
I found a solution:
assert isinstance(obs_space, gym.spaces.Box)
dtype: type[np.integer] | type[np.floating]
if np.issubdtype(obs_space.dtype, np.floating):
dtype = np.float32
elif np.issubdtype(obs_space.dtype, np.integer):
dtype = np.int32
assert dtype is not None, "dtype for `Box` space must be type[floating[Any]] | type[integer[Any]]"
self.observation_space = gym.spaces.Box(
low=np.min(obs_space.low),
high=np.max(obs_space.high),
shape=(self.size, self.size),
dtype=dtype,
)
One concern I have is that there could be other suitable floating-point types (e.g., np.float64) based on the system.
Since this mypy-related code gets so complex, we might just add a type ignore.
There was a problem hiding this comment.
Yeah, I also feel that we shouldn't overwrite the dtype of the original observation space. What do you think @MischaPanch ?
There was a problem hiding this comment.
Sorry, it had escaped my attention. How about just replacing the assert by cast?
There was a problem hiding this comment.
Unfortunately casting the obs_space is weaker than the assert and mypy will complain that some attrs don't exist because it won't assume the space to be Box.
There was a problem hiding this comment.
I'll check and will push the fix directly to master. We need to find some way of testing the examples, I'm going to contact some people from AMD and ask them to donate us CI workers
There was a problem hiding this comment.
Ok, mind that this dtype issue is in WarpFrame and FrameStack.
There was a problem hiding this comment.
That was just a bug - the type was too much in the isinstance check. I factored it out into a function and fixed it :)
Closes thu-ml#952 - `SamplingConfig` supports `batch_size=None`. thu-ml#1077 - tests and examples are covered by `mypy`. thu-ml#1077 - `NetBase` is more used, stricter typing by making it generic. thu-ml#1077 - `utils.net.common.Recurrent` now receives and returns a `RecurrentStateBatch` instead of a dict. thu-ml#1077 --------- Co-authored-by: Michael Panchenko <m.panchenko@appliedai.de>
Closes thu-ml#952 - `SamplingConfig` supports `batch_size=None`. thu-ml#1077 - tests and examples are covered by `mypy`. thu-ml#1077 - `NetBase` is more used, stricter typing by making it generic. thu-ml#1077 - `utils.net.common.Recurrent` now receives and returns a `RecurrentStateBatch` instead of a dict. thu-ml#1077 --------- Co-authored-by: Michael Panchenko <m.panchenko@appliedai.de>
Closes thu-ml#952 - `SamplingConfig` supports `batch_size=None`. thu-ml#1077 - tests and examples are covered by `mypy`. thu-ml#1077 - `NetBase` is more used, stricter typing by making it generic. thu-ml#1077 - `utils.net.common.Recurrent` now receives and returns a `RecurrentStateBatch` instead of a dict. thu-ml#1077 --------- Co-authored-by: Michael Panchenko <m.panchenko@appliedai.de>
Closes thu-ml#952 - `SamplingConfig` supports `batch_size=None`. thu-ml#1077 - tests and examples are covered by `mypy`. thu-ml#1077 - `NetBase` is more used, stricter typing by making it generic. thu-ml#1077 - `utils.net.common.Recurrent` now receives and returns a `RecurrentStateBatch` instead of a dict. thu-ml#1077 --------- Co-authored-by: Michael Panchenko <m.panchenko@appliedai.de>
Closes #952
SamplingConfigsupportsbatch_size=None. Fix mypy issues in tests and examples #1077mypy. Fix mypy issues in tests and examples #1077NetBaseis more used, stricter typing by making it generic. Fix mypy issues in tests and examples #1077utils.net.common.Recurrentnow receives and returns aRecurrentStateBatchinstead of a dict. Fix mypy issues in tests and examples #1077