Partition MyPy based on interpreter constraints #10817

Eric-Arellano · 2020-09-19T01:47:10Z

In a mixed repository, such as some Py27-only code and some Py3 code, we partition based on interpreter constraints for Flake8, Bandit, and Pylint, which ensures that we run with the correct interpreter for each. This means, that Py2-only syntax and Py3-only syntax can co-exist at the same time.

This brings that feature to MyPy. It's a similar implementation, except that we must set --python-version ourselves to influence which AST is used. If the user already set this, we respect it but log a warning. We also must consider the transitive closure in order to determine the final resulting compatibility (the new test demonstrates why this is the case.)

Benchmark

We expect that running TransitiveTargets on every single input FieldSet is going to be expensive.

When running ./pants typecheck ::, using time.time() around the await Get block that resolves transitive deps:

Before, running only once for everything: 3.65
After, running once per input target: 5.41

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

src/python/pants/backend/python/lint/pylint/rules.py

cosmicexplorer · 2020-09-21T21:31:41Z

To overcome the issue of "my code is all compatible with py2 and py3, and I want to make sure mypy runs over both of those", here's an extraordinarily complex process:

if we do not already have a partition that is using py2 and a partition that is using py3 after the first compatibility partitioning,
then sweep over all the targets again, looking specifically for ones that are compatible with both py2 and py3.
create a new partition with those targets, adding a <3 to force it to resolve py2 or >2 to force it to resolve py3 (the one that wasn't yet chosen).

This is intended to make sure we can run both mypy 2 and 3 on all code that is compatible with both.

Eric-Arellano · 2020-09-25T00:41:23Z

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

+    *, config: Optional[FileContent], args: Tuple[str, ...]
+) -> bool:
+    ending = (
+        "Normally, Pants would automatically set this for you based on your code's interpreter "


Before, Pants would no matter what set python_version. But Danny and I realized it's important to allow you to override.

In the Pex repo, which is ==2.7 or >=3.5, we run MyPy both with Py2 and Py3 to ensure it all works. We need to allow users to still do that.

Before, Pants would no matter what set python_version. But Danny and I realized it's important to allow you to override.

Why is that? Shouldn't the constraints always be declared on the targets? My understanding of why this might be set was that someone might want to use mypy outside/independently of pants, in which case pants should still override it here I think.

This warning doesn't seem actionable... if there is a valid reason to set these options, then a user would need to figure out how to match it with a regex to silence it. So would add an explicit option that folks could set to disable the warning.

In the Pex repo, which is ==2.7 or >=3.5, we run MyPy both with Py2 and Py3 to ensure it all works. We need to allow users to still do that.

We want to allow users to run ./pants typecheck :: then ./pants typecheck :: --mypy-args='--py2' right after. It's really important that we do that in Pex because MyPy checks differently depending on which AST is used; something valid in Py2 mode might be illegal in Py3 mode. It's not enough to run with the minimimum_python_version.

Ack on not being actionable enough. It's more or less meant to be "You can do this, just might be undefined behavior." I don't know how I feel about an option. If we did that, I think we'd want the option to be --mypy-partition-by-interpreter-constraints, rather than --mypy-ignore-python-version-warning etc. But then that gets to the original conversation: do we want to feature gate this? Idk. I'm concerned you turn it off, then you need it without realizing, like I suspect would have happened with the Toolchain repo.

We want to allow users to run ./pants typecheck :: then ./pants typecheck :: --mypy-args='--py2' right after.

That doesn't seem like a desirable thing... wouldn't it be better to mark the targets as requiring both 2 and 3, and then have pants run them both in parallel?

Danny and I talked about that, but it proves really challenging to figure out what the user intended. Let's say your code is >=3.5. Surely you don't want MyPy to run once with Py35, once with Py36, once with Py37, etc. If you have 3.5-3.9 all installed, that means 4 runs over the same code.

Now throw in partitioning on top of it. You could end up running MyPy dozens of times on your codebase, when you didn't want that.

We reasoned that the best compromise is for Pants to do the sensible thing of defaulting to running with the minimum Python version, while still providing a safety valve that you can override it. If you weren't using Pants, you would likewise need to override it via sequential runs of MyPy.

Danny and I talked about that, but it proves really challenging to figure out what the user intended. Let's say your code is >=3.5. Surely you don't want MyPy to run once with Py35, once with Py36, once with Py37, etc. If you have 3.5-3.9 all installed, that means 4 runs over the same code.

You would only run dozens of times if there were dozens of distinct ranges in BUILD files... in which case that is probably what you actually intended to happen.

You would only run dozens of times if there were dozens of distinct ranges in BUILD files...

Negative, unless we're talking about something different. What I think you're proposing is that for code that's Py2 and Py3 compatible, Pants automatically runs over that same code twice, once with Py2 and once with Py3.

Now imagine our default constraints of >=3.6,<4 that we set for everyone. This means that we'd need to run 3.6, 3.7, 3.8, 3.9, 3.10, etc, over the exact same code. That's very bad default behavior.

Eric-Arellano · 2020-09-25T00:44:11Z

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

+    # When determining how to batch by interpreter constraints, we must consider the entire
+    # transitive closure to get the final resulting constraints.
+    transitive_targets_per_field_set = await MultiGet(
+        Get(TransitiveTargets, Addresses([field_set.address])) for field_set in request.field_sets
+    )


This is my main concern. It adds about 1.5 seconds to running MyPy on the Pants codebase. Performance is already an issue with Pants and MyPy.

I don't know if we should feature-gate partitioning feature.

Does this await MultiGet cause the 1.5 second slowdown, or the corresponding additional runs of mypy? I would expect that when running mypy over the entire pants codebase, that 1.5 seconds more wouldn't be too much to ask?

My benchmark was running this specific MulitGet one-fieldset-at-a-time vs. including all the field sets in the input Addresses for one single Get. I used time.time() to measure.

Idrk what the right answer is with feature gating. I think for sure we want to land this feature, as it's a really helpful capability. My concern with feature gating is that you might turn it off without realizing the need. For example, Toolchain used to be >=3.8 always. Recently, we made a portion of our codebase >=3.7. While it's not the end of the world if we use >=3.8 on that too, we actually do want the partitioning imo because it will mean MyPy can check that we're not using Py38 syntax on accident. I don't think we would have realized we should turn on this partitioning when making the change; it's subtle.

Feature gating also makes this code more complex, although that's not something we should optimize too much for imo.

On the other hand, 1.5 seconds when Pants is already super slow with MyPy isn't great.

it will mean MyPy can check that we're not using Py38 syntax on accident

Isn't that going to be checked just by running the code as well? It'd be much more annoying, obviously, but it wouldn't be a horribly silent error, I think.

Isn't that going to be checked just by running the code as well

Only if we actually run the code with Py37. For that particular code, we don't have many tests yet. And we don't yet do what is really robust: creating Py37 and Py38 targets for the same test file to ensure it passes with both interpreters. (Or, doing what Pants does to run nightly CI by changing the global default).

So, MyPy running with Py37 is a simple, automated thing that would catch issues in the meantime. Along with catching type issues that tests might not catch if they aren't comprehensive enough.

Given the above, I would personally opt for taking the 1.5 second hit to avoid silent errors (especially since feature gating would directly affect toolchain). I think there are likely to be other places to improve performance, after profiling, if that becomes even mildly annoying.

I bias towards the correctness argument, too. You don't need partitioning Until You Do.

I think Stu had some ideas for how we might be able to make the calls to TransitiveTargets faster. And I didn't benchmark using pantsd, so it might be less offensive there.

But still, 1.5 seconds is substantial when the purpose of our tool is to speed up your workflow.

But still, 1.5 seconds is substantial when the purpose of our tool is to speed up your workflow.

Avoiding the need to manually construct the mypy command line is a reduction in mental energy which (along with not having to construct the same for pytest, etc) would be the tradeoff we can intentionally offer (if we find mypy is stubbornly difficult to speed up).

Yes, I think that we should land this and partition by default. Allowing for "compute the transitive targets for all of these roots" should be a relatively simple API change to TT to preserve the graph structure in the result, and then expose methods to get the closure on a root-by-root basis.

Adding a TODO here, and a few other comments in the body of this method would be good though.

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

This fixes a unit test. # 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]

# 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]

coveralls · 2020-09-25T02:09:10Z

Coverage remained the same at 0.0% when pulling 9dc0981 on Eric-Arellano:mypy-partition into ee98ef6 on pantsbuild:master.

cosmicexplorer

I don't think there's anything too blocking here although I would like to avoid printing out the ending in mypy/rules.py more than once, if possible, and adding the constant for max patch version. But I think the 1.5 second slowdown you identified is likely to be the more significant blocker, if it is one.

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

src/python/pants/backend/python/util_rules/pex.py

src/python/pants/backend/python/util_rules/pex_test.py

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

cosmicexplorer

Feel free to make the decision on whether to print ending more than once and whether to feature-gate the partitioning!

stuhood

Thanks!

stuhood · 2020-09-25T17:36:24Z

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

+    *, config: Optional[FileContent], args: Tuple[str, ...]
+) -> bool:
+    ending = (
+        "Normally, Pants would automatically set this for you based on your code's interpreter "


Before, Pants would no matter what set python_version. But Danny and I realized it's important to allow you to override.

Why is that? Shouldn't the constraints always be declared on the targets? My understanding of why this might be set was that someone might want to use mypy outside/independently of pants, in which case pants should still override it here I think.

This warning doesn't seem actionable... if there is a valid reason to set these options, then a user would need to figure out how to match it with a regex to silence it. So would add an explicit option that folks could set to disable the warning.

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

stuhood · 2020-09-25T17:44:24Z

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

+    # When determining how to batch by interpreter constraints, we must consider the entire
+    # transitive closure to get the final resulting constraints.
+    transitive_targets_per_field_set = await MultiGet(
+        Get(TransitiveTargets, Addresses([field_set.address])) for field_set in request.field_sets
+    )


Adding a TODO here, and a few other comments in the body of this method would be good though.

* magic numbers * talk about MYPYPATH * try to make warning more actionable * typo * ensure we only log one time [ci skip-rust] [ci skip-build-wheels]

cosmicexplorer reviewed Sep 21, 2020

View reviewed changes

src/python/pants/backend/python/lint/pylint/rules.py Outdated Show resolved Hide resolved

cosmicexplorer mentioned this pull request Sep 22, 2020

Fix MyPy with Python 2-only third-party requirements #10820

Merged

Partition MyPy based on interpreter constraints

7bf5e77

Eric-Arellano force-pushed the mypy-partition branch from 0c9f8fb to 7bf5e77 Compare September 25, 2020 00:20

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

Eric-Arellano changed the title ~~WIP: Partition MyPy based on interpreter constraints~~ Partition MyPy based on interpreter constraints Sep 25, 2020

Eric-Arellano requested review from cosmicexplorer, stuhood and benjyw September 25, 2020 00:39

Eric-Arellano commented Sep 25, 2020

View reviewed changes

Eric-Arellano added 3 commits September 24, 2020 17:49

Force CI

eb6a177

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

Bump default interpreter constraints to >=3.6

d8c9f5e

This fixes a unit test. # 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]

Remove some unused PexInterpreterConstraints options

9cebfa1

# 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 reviewed Sep 25, 2020

View reviewed changes

cosmicexplorer approved these changes Sep 25, 2020

View reviewed changes

stuhood approved these changes Sep 25, 2020

View reviewed changes

Review feedback:

9dc0981

* magic numbers * talk about MYPYPATH * try to make warning more actionable * typo * ensure we only log one time [ci skip-rust] [ci skip-build-wheels]

Eric-Arellano merged commit 905f849 into pantsbuild:master Sep 25, 2020

Eric-Arellano deleted the mypy-partition branch September 25, 2020 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partition MyPy based on interpreter constraints #10817

Partition MyPy based on interpreter constraints #10817

Eric-Arellano commented Sep 19, 2020 •

edited

cosmicexplorer commented Sep 21, 2020

Eric-Arellano Sep 25, 2020

stuhood Sep 25, 2020 •

edited

Eric-Arellano Sep 25, 2020

stuhood Sep 25, 2020 •

edited

Eric-Arellano Sep 25, 2020

stuhood Sep 25, 2020

Eric-Arellano Sep 25, 2020

Eric-Arellano Sep 25, 2020

cosmicexplorer Sep 25, 2020

Eric-Arellano Sep 25, 2020

cosmicexplorer Sep 25, 2020

Eric-Arellano Sep 25, 2020

cosmicexplorer Sep 25, 2020

Eric-Arellano Sep 25, 2020

cosmicexplorer Sep 25, 2020

stuhood Sep 25, 2020

stuhood Sep 25, 2020

coveralls commented Sep 25, 2020 •

edited

cosmicexplorer left a comment

cosmicexplorer left a comment

stuhood left a comment

stuhood Sep 25, 2020 •

edited

stuhood Sep 25, 2020

Partition MyPy based on interpreter constraints #10817

Partition MyPy based on interpreter constraints #10817

Conversation

Eric-Arellano commented Sep 19, 2020 • edited

Benchmark

cosmicexplorer commented Sep 21, 2020

Choose a reason for hiding this comment

stuhood Sep 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Sep 25, 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

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

coveralls commented Sep 25, 2020 • edited

cosmicexplorer left a comment

Choose a reason for hiding this comment

cosmicexplorer left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood Sep 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Sep 19, 2020 •

edited

stuhood Sep 25, 2020 •

edited

stuhood Sep 25, 2020 •

edited

coveralls commented Sep 25, 2020 •

edited

stuhood Sep 25, 2020 •

edited