Add support for `.pyi` type stubs (including for first-party) #10759

Eric-Arellano · 2020-09-11T03:12:37Z

There are two use cases for writing .pyi type stubs:

Writing bindings for third party libraries.
Writing stubs for your own code because you prefer it over using type hints/comments.

This PR adds support for both use cases. We allow Python targets to now include .pyi files, and update the default globs for python_library and python_tests to include them.

We also teach dependency inference about .pyi files. For third party reqs, we infer a dep on both the req and the .pyi file. For first party stubs, we infer a dep on both the .py and .pyi files.

Most of our Python functionality works correctly without changes for .pyi files, but we have to teach Pytest to not try running over .pyi files if they show up in python_tests targets, just like we special case conftest.py. We also update how we call MyPy; if there is a .pyi file and .py file, the .pyi file takes precedence; otherwise, MyPy would error.

[ci skip-rust]
[ci skip-build-wheels]

Eric-Arellano · 2020-09-15T23:03:29Z

Hey @cristianmatache, if you have the time, it'd be helpful to add tests that Black, Docformatter, and Isort all correctly format .pyi files. I don't think you'll need to make changes to the actual implementations; the tests are only for us to ensure we do the right thing.

First, check out https://www.pantsbuild.org/v2.0/docs/contributor-overview and then https://www.pantsbuild.org/v2.0/docs/contributor-setup for how to get set up. Warning that compiling Rust is slow, but cached.

Then, add a new test to each of {black,isort,docformatter}/rules_integration_test.py. The tests will all be extremely similar to

pants/src/python/pants/backend/python/lint/isort/rules_integration_test.py

Lines 102 to 121 in 7ac6480

    
           def test_passing_source(rule_runner: RuleRunner) -> None: 
        
               target = make_target_with_origin(rule_runner, [GOOD_SOURCE]) 
        
               lint_results, fmt_result = run_isort(rule_runner, [target]) 
        
               assert len(lint_results) == 1 
        
               assert lint_results[0].exit_code == 0 
        
               assert lint_results[0].stderr == "" 
        
               assert fmt_result.stdout == "" 
        
               assert fmt_result.output == get_digest(rule_runner, [GOOD_SOURCE]) 
        
               assert fmt_result.did_change is False 
        
           def test_failing_source(rule_runner: RuleRunner) -> None: 
        
               target = make_target_with_origin(rule_runner, [BAD_SOURCE]) 
        
               lint_results, fmt_result = run_isort(rule_runner, [target]) 
        
               assert len(lint_results) == 1 
        
               assert lint_results[0].exit_code == 1 
        
               assert "bad.py Imports are incorrectly sorted" in lint_results[0].stderr 
        
               assert fmt_result.stdout == "Fixing bad.py\n" 
        
               assert fmt_result.output == get_digest(rule_runner, [FIXED_BAD_SOURCE]) 
        
               assert fmt_result.did_change is True

I think everything will be the same, except you'll set up a different file that ends in .pyi rather than .py. You might want to do a "failing" file, as that's better verification that the tool is actually doing something.

Doesn't need to be super exhaustive - testing one file should be adequate, and avoids slowing down CI too much.

Although, maybe we should do a more robust test that includes a .py and .pyi file for the same module? Ensure that it doesn't trip up the formatters? I see no reason why it would - they don't care if two files are the same module. But maybe a good idea, regardless.

Feel free to push directly to this branch! (Lmk if I need to give you permissions.)

DONE: * Loosen target types * Teach Pytest to skip type stubs * Update dep inference TODO: * Add tests to MyPy [ci skip-rust] [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

src/python/pants/backend/python/dependency_inference/module_mapper.py

Eric-Arellano · 2020-09-26T00:55:58Z

src/python/pants/backend/python/goals/setup_py_test.py

@@ -129,20 +129,20 @@ def test_generate_chroot(chroot_rule_runner: RuleRunner) -> None:
        "src/python/foo/bar/baz",
        textwrap.dedent(
            """
-        python_distribution(


This only fixes indentation.

Eric-Arellano · 2020-09-26T00:58:15Z

src/python/pants/backend/python/target_types.py

+        "test_*.py",
+        "*_test.py",
+        "tests.py",
+        "conftest.py",
+        "test_*.pyi",
+        "*_test.pyi",
+        "tests.pyi",


I don't love this. But I think it's important to include .pyi in python_tests too. If a user for some reason writes type stubs for their tests, then we don't want it to show up in, say, setup-py. That is, we don't want the test type stubs defaulting to a python_library.

(Even though it's really weird to write a type stub file for your test)

I don't love this.

Is it just the number of lines to express it? Or you don't like how much it globs?

I might recommend putting the .pyi globs right after the .py globs, I think it might make it easier to read. But it's fine either way.

I don't like how many globs we now have. But I do think that this is the correct behavior. Just clunky.

Eric-Arellano · 2020-09-26T01:00:54Z

This still needs tests for the formatters, Flake8, and Pylint, which Cristian and I working on. But otherwise, this is good to go.

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano

Thank you Cristian!

src/python/pants/backend/python/lint/black/rules_integration_test.py

Eric-Arellano · 2020-09-27T20:32:55Z

src/python/pants/backend/python/lint/black/rules_integration_test.py

@@ -1,6 +1,6 @@
 # Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).


(Other reviewers: credit to @cristianmatache for writing this file. Thank you!)

Eric-Arellano · 2020-09-27T20:33:13Z

src/python/pants/backend/python/lint/isort/rules_integration_test.py

@@ -1,6 +1,7 @@
 # Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).


(Ditto on Cristian adding this change)

src/python/pants/backend/python/lint/isort/rules_integration_test.py

coveralls · 2020-09-27T21:04:35Z

Coverage remained the same at 0.0% when pulling 8657bfc on Eric-Arellano:python-type-stubs into f968daf on pantsbuild:master.

cosmicexplorer

I'm leaving it as "comment" in case you want to make any of the changes e.g. to add any of the comments I suggested, but to be clear, I think you can safely consider all of these suggestions as mostly optional. I tried to be useful and precise with the text suggestions, but those should also be optional. Once you mark each of them as resolved I will approve.

I wasn't trying to be pedantic here, but I am very unfamiliar with how type stubs work and only a little more familiar with dependency inference. Anything that seems like it should be common enough knowledge, you can feel free to ignore.

src/python/pants/backend/python/dependency_inference/module_mapper.py

cosmicexplorer · 2020-09-27T20:42:53Z

src/python/pants/backend/python/dependency_inference/module_mapper.py

+    def addresses_for_module(self, module: str) -> Tuple[Address, ...]:
+        targets = self.mapping.get(module)
+        if targets:
+            return targets
        # If the module is not found, try the parent, if any. This is to accommodate `from`
        # imports, where we don't care about the specific symbol, but only the module. For example,
        # with `from typing import List`, we only care about `typing`.
        # Unlike with third party modules, we do not look past the direct parent.


We say "unlike with third party modules", but then provide an example with typing above -- could we provide a more relevant example? It's also not clear to me why the parent module is only significant for from imports (wouldn't we need to check the parent for import pants.x.y too?).

src/python/pants/backend/python/dependency_inference/module_mapper.py

src/python/pants/backend/python/lint/black/rules_integration_test.py

cosmicexplorer · 2020-09-27T21:03:23Z

src/python/pants/backend/python/target_types.py

+        "test_*.py",
+        "*_test.py",
+        "tests.py",
+        "conftest.py",
+        "test_*.pyi",
+        "*_test.pyi",
+        "tests.pyi",


I don't love this.

Is it just the number of lines to express it? Or you don't like how much it globs?

I might recommend putting the .pyi globs right after the .py globs, I think it might make it easier to read. But it's fine either way.

cosmicexplorer · 2020-09-27T21:04:18Z

src/python/pants/backend/python/typecheck/mypy/rules.py

@@ -119,6 +119,25 @@ def config_path_globs(mypy: MyPy) -> PathGlobs:
    )


+def determine_python_files(files: Iterable[str]) -> Tuple[str, ...]:
+    """We run over all .py and .pyi files, but .pyi files take precedence.


Sorry, why is this? Why can we do py_file = f[:-1] below?

Why can we do py_file = f[:-1] below?

This gets the filename with the i removed.

Sorry, why is this?

MyPy will error if we say to run over the same module with both its .py and .pyi files, so we
must be careful to only use the .pyi stub.

MyPy only lets you choose one. Type stubs are supposed to take precedence over implementation files.

src/python/pants/backend/python/typecheck/mypy/rules_integration_test.py

…place # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano

Thanks for the great suggestions, Danny!

src/python/pants/backend/python/dependency_inference/module_mapper.py

Eric-Arellano · 2020-09-28T16:45:55Z

src/python/pants/backend/python/dependency_inference/module_mapper.py

+    return FirstPartyModuleToAddressMapping(
+        FrozenDict(
+            {
+                module: tuple(sorted(addresses))


Yes, that's correct. sorted() returns a list, so we have to convert into a hashable/immutable type.

Eric-Arellano · 2020-09-28T16:54:08Z

src/python/pants/backend/python/dependency_inference/module_mapper_test.py

+    # requirement. Note that `colors.pyi` is at the top-level of the source root so that it strips
+    # to the module `colors`.
+    rule_runner.create_file("source_root1/colors.pyi")
+    rule_runner.add_to_build_file("source_root1", "python_library()\n")


Doesn't look like it. It would be necessary if we wanted to call this method again on the same BUILD file though.

Eric-Arellano · 2020-09-28T16:56:42Z

src/python/pants/backend/python/dependency_inference/rules_test.py

@@ -51,6 +51,7 @@ def test_infer_python_imports() -> None:
    )

    rule_runner.create_file("src/python/str_import/subdir/f.py")
+    rule_runner.create_file("src/python/str_import/subdir/f.pyi")


Yes, particularly when we have .py and .pyi for the same module name. I can add a comment.

src/python/pants/backend/python/lint/black/rules_integration_test.py

Eric-Arellano · 2020-09-28T17:01:17Z

src/python/pants/backend/python/target_types.py

+        "test_*.py",
+        "*_test.py",
+        "tests.py",
+        "conftest.py",
+        "test_*.pyi",
+        "*_test.pyi",
+        "tests.pyi",


I don't like how many globs we now have. But I do think that this is the correct behavior. Just clunky.

Eric-Arellano · 2020-09-28T17:02:12Z

src/python/pants/backend/python/typecheck/mypy/rules.py

@@ -119,6 +119,25 @@ def config_path_globs(mypy: MyPy) -> PathGlobs:
    )


+def determine_python_files(files: Iterable[str]) -> Tuple[str, ...]:
+    """We run over all .py and .pyi files, but .pyi files take precedence.


Why can we do py_file = f[:-1] below?

This gets the filename with the i removed.

Eric-Arellano · 2020-09-28T17:02:55Z

src/python/pants/backend/python/typecheck/mypy/rules.py

@@ -119,6 +119,25 @@ def config_path_globs(mypy: MyPy) -> PathGlobs:
    )


+def determine_python_files(files: Iterable[str]) -> Tuple[str, ...]:
+    """We run over all .py and .pyi files, but .pyi files take precedence.


Sorry, why is this?

MyPy will error if we say to run over the same module with both its .py and .pyi files, so we
must be careful to only use the .pyi stub.

MyPy only lets you choose one. Type stubs are supposed to take precedence over implementation files.

[ci skip-rust] [ci skip-build-wheels]

Eric-Arellano · 2020-09-28T17:41:19Z

All the tests are now added: this is now ready for final reviews. Thanks!

stuhood

Looks great, thanks!

Too much copy pasta [ci skip-rust] [ci skip-build-wheels]

Eric-Arellano changed the title ~~WIP: Add support for .pyi type stubs~~ WIP: Add support for .pyi type stubs (including for first-party) Sep 11, 2020

Add support for .pyi type stubs

c36028e

DONE: * Loosen target types * Teach Pytest to skip type stubs * Update dep inference TODO: * Add tests to MyPy [ci skip-rust] [ci skip-build-wheels]

Eric-Arellano force-pushed the python-type-stubs branch from e2b9dcc to c36028e Compare September 22, 2020 04:51

initial attempt to include pyi and py in integration tests

8c629f9

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano force-pushed the python-type-stubs branch from c36028e to 1e6467b Compare September 26, 2020 00:31

Eric-Arellano changed the title ~~WIP: Add support for .pyi type stubs (including for first-party)~~ Add support for .pyi type stubs (including for first-party) Sep 26, 2020

Eric-Arellano marked this pull request as ready for review September 26, 2020 00:50

Add support for .pyi type stubs

a5fb27a

[ci skip-rust] [ci skip-build-wheels]

Eric-Arellano force-pushed the python-type-stubs branch from 1e6467b to a5fb27a Compare September 26, 2020 00:59

Eric-Arellano commented Sep 26, 2020

View reviewed changes

Eric-Arellano requested review from stuhood, benjyw and cosmicexplorer September 26, 2020 01:00

cristianmatache added 5 commits September 26, 2020 10:26

inlined creation of pyi file

214c3d5

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

synced with Eric

ff7773a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

test pyi files

8b299c6

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

rules integration test for black

bd08c6d

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

black

9236b21

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano commented Sep 27, 2020

View reviewed changes

cosmicexplorer reviewed Sep 27, 2020

View reviewed changes

removed extraneous tests and used qualified import for dataclasses.re…

566730c

…place # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

cosmicexplorer approved these changes Sep 28, 2020

View reviewed changes

Eric-Arellano commented Sep 28, 2020

View reviewed changes

Eric-Arellano added 2 commits September 28, 2020 10:30

Review feedback

66deda7

[ci skip-rust] [ci skip-build-wheels]

Add tests to Flake8, Pylint, and Docformatter

24f9a3b

[ci skip-rust] [ci skip-build-wheels]

stuhood approved these changes Sep 28, 2020

View reviewed changes

Fix docformatter

8657bfc

Too much copy pasta [ci skip-rust] [ci skip-build-wheels]

Eric-Arellano merged commit f90b982 into pantsbuild:master Sep 28, 2020

Eric-Arellano deleted the python-type-stubs branch September 28, 2020 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `.pyi` type stubs (including for first-party) #10759

Add support for `.pyi` type stubs (including for first-party) #10759

Eric-Arellano commented Sep 11, 2020 •

edited

Eric-Arellano commented Sep 15, 2020 •

edited

Eric-Arellano Sep 26, 2020

Eric-Arellano Sep 26, 2020

cosmicexplorer Sep 27, 2020

Eric-Arellano Sep 28, 2020

Eric-Arellano commented Sep 26, 2020

Eric-Arellano left a comment

Eric-Arellano Sep 27, 2020

Eric-Arellano Sep 27, 2020

coveralls commented Sep 27, 2020 •

edited

cosmicexplorer left a comment •

edited

cosmicexplorer Sep 27, 2020

cosmicexplorer Sep 27, 2020

cosmicexplorer Sep 27, 2020

Eric-Arellano Sep 28, 2020

Eric-Arellano Sep 28, 2020

Eric-Arellano left a comment

Eric-Arellano Sep 28, 2020

Eric-Arellano Sep 28, 2020

Eric-Arellano Sep 28, 2020

Eric-Arellano Sep 28, 2020

Eric-Arellano Sep 28, 2020

Eric-Arellano Sep 28, 2020

Eric-Arellano commented Sep 28, 2020

stuhood left a comment

		@@ -1,6 +1,6 @@
		# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).

		@@ -1,6 +1,7 @@
		# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).

Add support for .pyi type stubs (including for first-party) #10759

Add support for .pyi type stubs (including for first-party) #10759

Conversation

Eric-Arellano commented Sep 11, 2020 • edited

Eric-Arellano commented Sep 15, 2020 • edited

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

Eric-Arellano commented Sep 26, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 27, 2020 • edited

cosmicexplorer left a comment • edited

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

Eric-Arellano left a 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

Eric-Arellano commented Sep 28, 2020

stuhood left a comment

Choose a reason for hiding this comment

Add support for `.pyi` type stubs (including for first-party) #10759

Add support for `.pyi` type stubs (including for first-party) #10759

Eric-Arellano commented Sep 11, 2020 •

edited

Eric-Arellano commented Sep 15, 2020 •

edited

coveralls commented Sep 27, 2020 •

edited

cosmicexplorer left a comment •

edited