Pants: Fix MyPy to not directly run over dependencies

Created on 12 Feb 2021 · 2Comments · Source: pantsbuild/pants

Right now, we instruct MyPy to run over not only the specified files, but all transitive deps. A user found that this results in type checking more than the user actually intended. It is true that we need all transitive deps to be in the chroot, but we shouldn't be telling MyPy to run directly on those.

FYI, how it works now

We partition MyPy based on interpreter constraints, which allows for you to run over a codebase that might have some parts only Py2 and another part only Py3:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L60-L65

All the interesting work happens on partitions, in this rule:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L184-L185

We get all the source files for the partition here, which will continue to be the same so that we can include the files in our chroot:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L225-L227

What will change is this:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L278-L282

We won't run on all of typechecked_sources anymore, only what was specified. (FYI, see https://www.pantsbuild.org/docs/rules-api-file-system#core-abstractions-digest-and-snapshot for what the "snaphsot" part means.)

"roots" vs. "closure"

Instead, we need to figure out which files were specified by the user. How? Looking back at the below definition, the field_set_addresses represent the _roots_, meaning what the user explicitly said to run on. Meanwhile, closure represents the transitive closure: all roots + transitive deps.

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L60-L65

We want to operate on the roots, i.e. the field_set_addresses. We need to get the Python source files that belong specifically to those roots, whereas now we're getting it for the whole closure. We'll use the same approach as before, sayingGet(PythonSourceFiles, PythonSourceFilesRequest(..)), which will run a rule that gets us the relevant Python source files; only, we'll run it over just the roots, rather than the whole closure.

Part 1: refactor

First, there's a refactor to facilitate this change. Rather than storing on MyPyPartition the field field_set_addresses: FrozenOrderedSet[Address], change it to root_targets: FrozenOrderedSet[Target]. We're going to need Target objects in a moment, and this is also a clearer name.

Then, fix our rule that creates the partitions. Update to combined_roots.update(transitive_targets.roots):

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L370-L372

You'll need to fix this too, use tgt.address for tgt in partitions.root_targets to convert from Target -> Address:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L243

Check that everything works by running ./pants typecheck src/python/pants/util/strutil.py.

Part 2: resolve the roots' files

See this:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L222-L227

First, that name typechecked_sources_request is confusing now. Really, it should probably be something like closure_sources_request. We're not going to typecheck every one of those files anymore.

Then, add something like roots_sources_request = Get(PythonSourceFiles, PythonSourceFilesRequest(partition.root_targets)). Feel free to use a better var name.

Add to this await MultiGet, which is what will cause the engine to actually execute the relevant code:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L264-L276

Finally, update this to point to your new PythonSourceFiles object with just the roots, rather than the original one with the closure.

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L278

Update this:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L321

That should be it! Check that things are working how you want. It can be helpful to add logging statements to debug. https://www.pantsbuild.org/docs/rules-api-tips#tip-add-logging

This function will still stay the same to normalize file names, but the docstring should be tweaked as we won't run over every file anymore:

https://github.com/pantsbuild/pants/blob/9954e08966eb49f6e1996043bf2b90bad032c7f7/src/python/pants/backend/python/typecheck/mypy/rules.py#L119-L135

Testing

You'll also want to check that mypy/rules_integration_test.py works correctly still. You may need to tweak some tests.

It'd be helpful to add a simple test that ensures this behavior works correctly. We're going to be changing this MyPy code in the future and we don't want to regress. Look at how the other tests are doing it to see how to set up, and also refer to https://www.pantsbuild.org/docs/rules-api-testing.

help wanted

Source