Improve mypy coverage by adding --namespace-packages #3049
Conversation
| @@ -494,7 +495,7 @@ def _get_state(self, output: UnityRLOutputProto) -> AllBrainInfo: | |||
|
|
|||
| @staticmethod | |||
| def _parse_side_channel_message( | |||
| side_channels: Dict[int, SideChannel], data: bytearray | |||
| side_channels: Dict[int, SideChannel], data: bytes | |||
There was a problem hiding this comment.
bytes is immutable - these seemed better for processing inputs. I left outputs as bytearrays though.
| agent_info_list: List[AgentInfoProto], | ||
| agent_info_list: Collection[ | ||
| AgentInfoProto | ||
| ], # pylint: disable=unsubscriptable-object |
There was a problem hiding this comment.
See pylint-dev/pylint#3292 :sigh:
| return self._float_properties.keys() | ||
| return list(self._float_properties.keys()) | ||
|
|
||
| def get_property_dict(self) -> Dict[str, float]: |
There was a problem hiding this comment.
Add this since the usage pattern was always list_properties then get_property on each.
There was a problem hiding this comment.
Do we still need get and list properties then?
Also, it is not clear that modifying this dict will not modify the unity environment...
There was a problem hiding this comment.
Not sure we need list. I think get() is still useful.
Would get_property_dict_copy() make it clearer? Unfortunately there's no clean way to return a read-only dictionary.
| @@ -28,7 +28,7 @@ def reset_local_buffers(self) -> None: | |||
| def append_to_update_buffer( | |||
| self, | |||
| update_buffer: AgentBuffer, | |||
| agent_id: str, | |||
| agent_id: int, | |||
There was a problem hiding this comment.
@ervteng Is this correct now? mypy was complaining about demo_loader calling this with 0 instead of a string.
There was a problem hiding this comment.
Demo loader is wrong, it uses an int for the update buffer. However in env_manager that int is turned into a string, so the int won't work in the general case.
There was a problem hiding this comment.
Actually it's both, in different parts of the code :P
There was a problem hiding this comment.
Made it a Union[int, str] for now
| return self._float_properties.keys() | ||
| return list(self._float_properties.keys()) | ||
|
|
||
| def get_property_dict(self) -> Dict[str, float]: |
There was a problem hiding this comment.
Do we still need get and list properties then?
Also, it is not clear that modifying this dict will not modify the unity environment...
| self.all_log_probs: Optional[tf.Tensor] = None | ||
| self.output: Optional[tf.Tensor] = None | ||
| self.selected_actions: Optional[tf.Tensor] = None | ||
| self.action_holder: Optional[tf.Tensor] = None |
There was a problem hiding this comment.
Did you intend to add those? It does not seem related to mypy coverage.
There was a problem hiding this comment.
Yes, there were places where mypy was complaining (correctly) that e.g. all_log_probs wasn't a member of LearningModel. In practice it was OK because they were always defined in PPOModel or SACModel. This just defines them in the base class too.
| import numpy as np | ||
| from enum import Enum | ||
|
|
||
| AgentId = NewType("AgentId", int) | ||
| AgentGroup = NewType("AgentGroup", str) | ||
| AgentId = int |
There was a problem hiding this comment.
I think this was going to be more trouble that it was worth. This still aliases the types so that it's a little clearer than the raw int/str types.
| @@ -371,8 +372,8 @@ def set_action_for_agent( | |||
| action = action.astype(expected_type) | |||
|
|
|||
| if agent_group not in self._env_actions: | |||
| self._env_actions[agent_group] = self._empty_action( | |||
| spec, self._env_state[agent_group].n_agents() | |||
| self._env_actions[agent_group] = spec.create_empty_action( | |||
chriselion
changed the title
Turning on --namespace-packages gets mypy to recognize more types across files. Unfortunately it also triggers an assert in mypy, because it ends up reporting the same file twice (see python/mypy#7510).
I haven't seen any progress on the mypy issue, so I forked, disabled the assertion, and set up precommit to look at the fork instead.
This PR fixes the errors uncovered by doing this.