[TorchMultimodal][refactor] Support type checking with mypy #22

ebsmothers · 2022-04-08T18:54:39Z

Stack from ghstack (oldest at bottom):

-> [TorchMultimodal][refactor] Support type checking with mypy #22

Summary:
Adding support for type checking using mypy.
Cleaning up a bunch of files to fix existing type check errors.
Note that for now we are only type checking non-FLAVA code inside torchmultimodal.
Also adding some instructions to CONTRIBUTING.md

Test plan:

mypy
Success: no issues found in 28 source files

python3 -m pytest -vv
...
42 passed, 2 skipped, 26 warnings in 22.87s

Differential Revision: D35584888

Summary: Adding support for type checking using mypy. Cleaning up a bunch of files to fix existing type check errors. Note that for now we are only type checking non-FLAVA code inside torchmultimodal. Also adding some instructions to CONTRIBUTING.md Test plan: ``` mypy Success: no issues found in 28 source files ``` ``` python3 -m pytest -vv ... 42 passed, 2 skipped, 26 warnings in 22.87s ``` [ghstack-poisoned]

Summary: Adding support for type checking using mypy. Cleaning up a bunch of files to fix existing type check errors. Note that for now we are only type checking non-FLAVA code inside torchmultimodal. Also adding some instructions to CONTRIBUTING.md Test plan: ``` mypy Success: no issues found in 28 source files ``` ``` python3 -m pytest -vv ... 42 passed, 2 skipped, 26 warnings in 22.87s ``` ghstack-source-id: c7011c48f255d5816cc95f2c944629b3d15a32b0 Pull Request resolved: #22

langong347 · 2022-04-08T23:23:21Z

mypy.ini

+namespace_packages = True
+install_types = True
+
+exclude = models/flava.py|modules/losses/flava.py


Thanks! Mind filing a BE task on fixing FLAVA type annotation for tracking?

langong347 · 2022-04-08T23:27:33Z

mypy.ini

+exclude = models/flava.py|modules/losses/flava.py
+
+[mypy-PIL.*]
+ignore_missing_imports = True


Is this because pillow is installed with torchvision?
Wonder if we only need this for pytorch domain libs?

No this is true for PIL in general. Ref

ankitade · 2022-04-12T03:30:05Z

torchmultimodal/modules/encoders/clip_text_encoder.py

@@ -87,7 +87,7 @@ def initialize_parameters(self):

    def build_attention_mask(self):


no return annotation needed?

It is probably due to this property of mypy where functions without any annotations are ignored completely. Not sure why this is, but we should see if there is some solution for this behavior.

ankitade · 2022-04-12T03:32:49Z

torchmultimodal/transforms/text_transforms.py

@@ -30,7 +30,7 @@ def __call__(
        self, l: Union[List[str], List[List[str]]]
    ) -> Union[List[int], List[List[int]]]:
        if isinstance(l[0], str):
-            return [int(x) for x in l]
+            return [int(x) for x in l]  # type: ignore


why ignore for only this one?

There's an assumption here that every element of l is either str or List[str]. So we only check that the first one is a str. But then mypy complains because it thinks the other elements could still be List[str]. Personally I would rather ignore the error than iterate over the whole list just to make mypy happy

ankitade · 2022-04-12T03:33:42Z

CONTRIBUTING.md

+TorchMultimodal uses mypy for type checking. You can install mypy with the command
+
+```
+python3 -m pip install mypy


can u just do pip install like others?

Maybe, but this is from the mypy docs, so personally I would stick with the officially-supported command

ankitade · 2022-04-12T03:36:02Z

mypy.ini

+[mypy-PIL.*]
+ignore_missing_imports = True
+
+[mypy-torchtext.*]


why do we need these?

You can see the discussion here, but basically mypy expects to find stubs from any imported libraries, which are not present for the ones listed here. So the easiest approach is to just suppress them.

Summary: Adding support for type checking using mypy. Cleaning up a bunch of files to fix existing type check errors. Note that for now we are only type checking non-FLAVA code inside torchmultimodal. Also adding some instructions to CONTRIBUTING.md Test plan: ``` mypy Success: no issues found in 28 source files ``` ``` python3 -m pytest -vv ... 42 passed, 2 skipped, 26 warnings in 22.87s ``` [ghstack-poisoned]

Summary: Adding support for type checking using mypy. Cleaning up a bunch of files to fix existing type check errors. Note that for now we are only type checking non-FLAVA code inside torchmultimodal. Also adding some instructions to CONTRIBUTING.md Test plan: ``` mypy Success: no issues found in 28 source files ``` ``` python3 -m pytest -vv ... 42 passed, 2 skipped, 26 warnings in 22.87s ``` ghstack-source-id: 0a2b5cea06b1d12a6e494c2ee66b148a788839c3 Pull Request resolved: #22

ebsmothers · 2022-04-12T17:58:16Z

@ebsmothers has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: A couple follow-ups from #22: - Add TODO for type checking FLAVA code - Add a couple other type hints missed by mypy in clip_text_encoder (due to https://mypy.readthedocs.io/en/stable/common_issues.html#no-errors-reported-for-obviously-wrong-code) - Some changes to weighted_embedding_encoder to silence mypy errors Test plan: ``` $ python -m pytest -v ... ====================================================================================== short test summary info ======================================================================================= FAILED test/transforms/test_clip_transform.py::TestCLIPTransform::test_clip_multi_transform - requests.exceptions.MissingSchema: Invalid URL '/data/home/ebs/torchmultimodal/torchmultimodal/test/a... FAILED test/transforms/test_clip_transform.py::TestCLIPTransform::test_clip_single_transform - requests.exceptions.MissingSchema: Invalid URL '/data/home/ebs/torchmultimodal/torchmultimodal/test/... ================================================================== 2 failed, 48 passed, 3 skipped, 26 warnings in 76.48s (0:01:16) =================================================================== ``` (Failures are pre-existing and will be fixed with a separate commit) ``` $ mypy Success: no issues found in 29 source files ``` [ghstack-poisoned]

Summary: A couple follow-ups from #22: - Add TODO for type checking FLAVA code - Add a couple other type hints missed by mypy in clip_text_encoder (due to https://mypy.readthedocs.io/en/stable/common_issues.html#no-errors-reported-for-obviously-wrong-code) - Some changes to weighted_embedding_encoder to silence mypy errors Test plan: ``` $ python -m pytest -v ... ====================================================================================== short test summary info ======================================================================================= FAILED test/transforms/test_clip_transform.py::TestCLIPTransform::test_clip_multi_transform - requests.exceptions.MissingSchema: Invalid URL '/data/home/ebs/torchmultimodal/torchmultimodal/test/a... FAILED test/transforms/test_clip_transform.py::TestCLIPTransform::test_clip_single_transform - requests.exceptions.MissingSchema: Invalid URL '/data/home/ebs/torchmultimodal/torchmultimodal/test/... ================================================================== 2 failed, 48 passed, 3 skipped, 26 warnings in 76.48s (0:01:16) =================================================================== ``` (Failures are pre-existing and will be fixed with a separate commit) ``` $ mypy Success: no issues found in 29 source files ``` ghstack-source-id: 1ec9d0468fb289d60ae269e79b7d8e1a17087920 Pull Request resolved: #25

Summary: Pull Request resolved: #25 A couple follow-ups from #22: - Add TODO for type checking FLAVA code - Add a couple other type hints missed by mypy in clip_text_encoder (due to https://mypy.readthedocs.io/en/stable/common_issues.html#no-errors-reported-for-obviously-wrong-code) - Some changes to weighted_embedding_encoder to silence mypy errors Test Plan: ``` $ python -m pytest -v ... ====================================================================================== short test summary info ======================================================================================= FAILED test/transforms/test_clip_transform.py::TestCLIPTransform::test_clip_multi_transform - requests.exceptions.MissingSchema: Invalid URL '/data/home/ebs/torchmultimodal/torchmultimodal/test/a... FAILED test/transforms/test_clip_transform.py::TestCLIPTransform::test_clip_single_transform - requests.exceptions.MissingSchema: Invalid URL '/data/home/ebs/torchmultimodal/torchmultimodal/test/... ================================================================== 2 failed, 48 passed, 3 skipped, 26 warnings in 76.48s (0:01:16) =================================================================== ``` (Failures are pre-existing and will be fixed with a separate commit) ``` $ mypy Success: no issues found in 29 source files ``` Reviewed By: langong347 Differential Revision: D35617460 Pulled By: ebsmothers fbshipit-source-id: ab2bf0ec7f85fa700287787728aa40443b9c7a0c

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 8, 2022

ebsmothers requested review from facebook-github-bot, langong347 and ankitade and removed request for facebook-github-bot April 8, 2022 20:49

langong347 approved these changes Apr 8, 2022

View reviewed changes

ankitade reviewed Apr 12, 2022

View reviewed changes

facebook-github-bot closed this in 63d183e Apr 13, 2022

ebsmothers mentioned this pull request Apr 13, 2022

[cleanup] Follow-ups from initial mypy PR #25

Closed

facebook-github-bot deleted the gh/ebsmothers/2/head branch April 16, 2022 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TorchMultimodal][refactor] Support type checking with mypy #22

[TorchMultimodal][refactor] Support type checking with mypy #22

ebsmothers commented Apr 8, 2022 •

edited

Loading

langong347 Apr 8, 2022

langong347 Apr 8, 2022

ebsmothers Apr 12, 2022

ankitade Apr 12, 2022

ebsmothers Apr 12, 2022

ankitade Apr 12, 2022

ebsmothers Apr 12, 2022

ankitade Apr 12, 2022

ebsmothers Apr 12, 2022

ankitade Apr 12, 2022

ebsmothers Apr 12, 2022

ebsmothers commented Apr 12, 2022

		@@ -87,7 +87,7 @@ def initialize_parameters(self):

		def build_attention_mask(self):

[TorchMultimodal][refactor] Support type checking with mypy #22

[TorchMultimodal][refactor] Support type checking with mypy #22

Conversation

ebsmothers commented Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebsmothers commented Apr 12, 2022

ebsmothers commented Apr 8, 2022 •

edited

Loading