Enable mypy in precommit checks #2177

chriselion · 2019-06-24T20:30:15Z

This enables mypy (python type annotation checks) for ml-agents, ml-agents-env, and gym-unity. The number of lines changed is a little intimidating, but it's mostly due to adding new stub files. I can break this into a PR per library though.

The repo layout added a few complications:

We need 3 separate invocations of mypy, filtered by directory
Currently need --ignore-missing-imports on each. Will try to remove those in subsequent PRs
The generated _pb2.py files are flagged as incorrect because the stubs https://github.com/python/typeshed/tree/master/third_party/2and3/google/protobuf don't have the serialized_options field that the current code does https://github.com/protocolbuffers/protobuf/blob/master/python/google/protobuf/descriptor.py#L539. Need to see if we can fix the stubs or if they're missing for a reason.
Because each directory is checked individually, I'm not sure that the type information from one is available in the other. Again, this can be improved in another PR.

…mypy

chriselion · 2019-06-25T17:18:39Z

ml-agents/mlagents/trainers/tests/test_trainer_controller.py

        tc.initialize_trainers(bad_config)
-        assert (


mypy correctly complained that the assert was always true. pytest.raises is a cleaner way to test that an exception is raised

chriselion · 2019-06-25T18:37:48Z

ml-agents-envs/mlagents/envs/brain.py

-        return l2.copy()
-    if l1 is not None and l2 is None:
-        return l1.copy()
+def safe_concat_lists(l1: Optional[List], l2: Optional[List]) -> Optional[List]:


mypy was getting confused about the null-ness of the inputs. Personally I think this is a little nicer anyway.

chriselion · 2019-06-25T18:38:40Z

ml-agents-envs/mlagents/envs/communicator_objects/agent_action_proto_pb2.pyi

@@ -0,0 +1,51 @@
+# @generated by generate_proto_mypy_stubs.py.  Do not edit!


All of these .pyi files are stubs generated with https://github.com/dropbox/mypy-protobuf. I updated the directions with how to generate them.

Why use stub files instead of putting the type annotation directly onto the python source file?

The python source here is generated by protoc and it's not generating traditional python classes. In theory we might be able to do it, but it would involve wrestling with both protoc and mypy-protobuf.

Oh I see, make sense.

chriselion · 2019-06-25T18:40:51Z

ml-agents-envs/mlagents/envs/environment.py

@@ -373,7 +373,18 @@ def step(
        custom_action = {} if custom_action is None else custom_action

        # Check that environment is loaded, and episode is currently running.
-        if self._loaded and not self._global_done and self._global_done is not None:
+        if not self._loaded:


It wasn't obvious that this block either raises or returns; I think this is a little cleaner.
Could also unindent the "main" block but trying to keep the diff smaller.

chriselion · 2019-06-25T18:51:20Z

ml-agents-envs/mlagents/envs/environment.py

@@ -623,7 +623,7 @@ def _get_state(self, output: UnityRLOutput) -> (AllBrainInfo, bool):

    def _generate_step_input(
        self, vector_action, memory, text_action, value, custom_action
-    ) -> UnityRLInput:
+    ) -> UnityInput:


These were just wrong :)

chriselion · 2019-06-25T18:51:52Z

ml-agents-envs/mlagents/envs/environment.py

@@ -607,7 +607,7 @@ def _flatten(cls, arr) -> List[float]:
        arr = [float(x) for x in arr]
        return arr

-    def _get_state(self, output: UnityRLOutput) -> (AllBrainInfo, bool):


Unfortunately that's not valid type annotation syntax, you have to use Tuple[]

chriselion · 2019-06-25T19:04:50Z

@ervteng just a heads up. I don't think this conflicts much with your rewards refactor but there might be a bit of overlap.

xiaomaogy · 2019-06-25T23:10:24Z

.pre-commit-config.yaml

+    -   id: mypy
+        name: mypy-ml-agents
+        files: "ml-agents/.*"
+        args: [--ignore-missing-imports, --disallow-incomplete-defs]


What are --ignore-missing-imports, --disallow-incomplete-defs used for?

--disallow-incomplete-defs means that the checker will complain about functions that have some arguments and return annotated, but not others. For example

def foo(x: int, y) -> int: # bad, no type hint on y def bar(x: int, y: int): # bad, no return type

would be treated as errors. I didn't apply it to ml-agents-env because there's still some code that I'm not sure what the types should be.

--ignore-missing-imports is a little more complicated; without it the checker will complain about importing numpy and tensorflow. It also complains about some imports in the mlagents repo; I think is related to our directory structure (similar to the package problems we had during the release) but I'll admit that I don't fully understand it yet. I would like to remove these in the future but it's the only practical way to get checks passing today.

xiaomaogy · 2019-06-25T23:11:18Z

CONTRIBUTING.md

@@ -48,6 +48,18 @@ the platform, and provide a unique non-trivial challenge to modern
 machine learning algorithms. Feel free to submit these environments with a
 PR explaining the nature of the environment and task.

-## Style Guide
+## Continuous Integration (CI)


I think this is still a style guide, we just enforce our style through CI, so maybe still keep this as Style Guide?

There's a "Code Style" section below. I wanted to give a little more explanation about how the checks (black, mypy, and anything else we add later) were run. I can change this back and move it to another doc if you think it was better before.

No I think everything is fine here. I don't have a strong opinion on this.

xiaomaogy · 2019-06-25T23:23:42Z

ml-agents-envs/mlagents/envs/rpc_communicator.py

@@ -94,7 +95,7 @@ def initialize(self, inputs: UnityInput) -> UnityOutput:
        self.unity_to_external.parent_conn.recv()
        return aca_param

-    def exchange(self, inputs: UnityInput) -> UnityOutput:
+    def exchange(self, inputs: UnityInput) -> Optional[UnityOutput]:


Why this is Optional here?

The method can potentially return None (line 105), so we have to change the return type. Optional[UnityOutput] is shorthand for Union[None, UnityOutput]

Oh I missed that part of the logic.

xiaomaogy · 2019-06-25T23:24:45Z

ml-agents/mlagents/trainers/demo_loader.py

@@ -4,7 +4,8 @@
 from mlagents.trainers.buffer import Buffer
 from mlagents.envs.brain import BrainParameters, BrainInfo
 from mlagents.envs.communicator_objects import *
-from google.protobuf.internal.decoder import _DecodeVarint32
+from google.protobuf.internal.decoder import _DecodeVarint32  # type: ignore


What does #type:ignore mean here?

_DecodeVarint32 isn't declared in the protobuf library stubs here: https://github.com/python/typeshed/blob/master/third_party/2and3/google/protobuf/internal/decoder.pyi
I'm not sure if this is an oversight, or because they don't want to provide type info for internal functions (it starts with a _), or because it's not available in all versions of protobuf.

# type: ignore just tells mypy to ignore errors on that line; it will treat _DecodeVarint32 as an Any. We can try to fix this up, but it didn't seem worth the effort.

I agree, this is good.

xiaomaogy · 2019-06-25T23:27:52Z

protobuf-definitions/make.bat

@@ -22,7 +22,7 @@ mkdir -p $DST_DIR_P/$PYTHON_PACKAGE
 # generate proto objects in python and C#

 protoc --proto_path=proto --csharp_out=$DST_DIR_C $SRC_DIR/*.proto
-protoc --proto_path=proto --python_out=$DST_DIR_P $SRC_DIR/*.proto 
+protoc --proto_path=proto --python_out=$DST_DIR_P --mypy_out=$DST_DIR_P $SRC_DIR/*.proto


Have you tested this?

Yes, that was how I generated the .pyi files.

Chris Elion added 17 commits June 21, 2019 08:54

WIP precommit on top level

3342f8a

update CI

8284924

circleci fixes

8e48831

intentionally fail black

f06cba6

use --show-diff-on-failure in CI

30298e0

fix command order

a84c05d

rebreak a file

f31aa33

apply black

9ae5869

WIP enable mypy

fd4f3bb

run mypy on each package

b49a455

fix trainer_metrics mypy errors

c3814bc

more mypy errors

e904f5d

more mypy

d3a1bc4

Fix some partially typed functions

6a4d282

types for take_action_outputs

094893e

fix formatting

0e79001

Merge remote-tracking branch 'origin/develop' into develop-precommit-…

1468c43

…mypy

chriselion commented Jun 25, 2019

View reviewed changes

Chris Elion added 3 commits June 25, 2019 10:41

cleanup

a32de87

generate stubs for proto objects

ad73974

fix ml-agents-env mypy errors

83269d2

chriselion commented Jun 25, 2019

View reviewed changes

disallow-incomplete-defs for gym-unity

ff8565d

chriselion commented Jun 25, 2019

View reviewed changes

chriselion changed the title ~~Develop precommit mypy~~ Enable mypy in precommit checks Jun 25, 2019

chriselion requested a review from xiaomaogy June 25, 2019 19:04

Add CI notes to CONTRIBUTING.md

3e90e0e

xiaomaogy reviewed Jun 25, 2019

View reviewed changes

xiaomaogy approved these changes Jun 25, 2019

View reviewed changes

chriselion merged commit 8b77f11 into develop Jun 25, 2019

chriselion deleted the develop-precommit-mypy branch July 11, 2019 22:26

github-actions bot locked as resolved and limited conversation to collaborators May 18, 2021

		@@ -0,0 +1,51 @@
		# @generated by generate_proto_mypy_stubs.py. Do not edit!

Enable mypy in precommit checks #2177

Enable mypy in precommit checks #2177

Uh oh!

Conversation

chriselion commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

chriselion commented Jun 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaomaogy Jun 25, 2019 • 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!

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!

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!

chriselion commented Jun 24, 2019 •

edited

Loading

xiaomaogy Jun 25, 2019 •

edited

Loading