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

Eric-Arellano · 2020-09-19T04:51:34Z

Turns out that when using --pex-path with PEX, it will expect for every requirement to have a wheel that is compatible with the current interpreter, unless you set PEX_IGNORE_ERRORS. This means that it is naively not safe to resolve requirements with Python 2, but run MyPy with Python 3.5+.

Likewise, it is not safe to resolve requirements with Python 3.6 but run MyPy with Python 3.7.

Instead, we generally use the same interpreter constraints for the requirements when creating the tool pex. However, if the requirements have any Python 2 code—or the user set --mypy-interpreter-constraints—then we will let the two sets diverge, and we'll set PEX_IGNORE_ERRORS.

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

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

…ents # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano · 2020-09-22T04:18:05Z

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

@@ -68,10 +68,7 @@ async def bandit_lint_partition(
            output_filename="bandit.pex",
            internal_only=True,
            requirements=PexRequirements(bandit.all_requirements),
-            interpreter_constraints=(
-                partition.interpreter_constraints
-                or PexInterpreterConstraints(bandit.interpreter_constraints)


The only situation where this could be triggered is if you didn't set compatibility anywhere and you went out of your way to set interpreter_constraints = [].

Imo, it's deceptive to have the option because almost always it has no impact on behavior.

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

Eric-Arellano · 2020-09-22T04:20:52Z

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

+    tool_interpreter_constraints = (
+        code_interpreter_constraints
+        if (
+            mypy.options.is_default("interpreter_constraints")


I spent a long time trying to figure out if this option should still exist. We might want to remove it when adding partitioning. I'm not sure yet.

Could we leave a TODO pointing to #10817?

I plan to finish that tomorrow, so I'd rather not if that's okay.

Okay I thought more about this and realized that partitioning is irrelevant to whether we have --mypy-interpreter-constraints or not. We can always allow users to set this.

But should we? If they set it, it's very possible that we will end up needing to set PEX_IGNORE_ERRORS, which may cause MyPy to complain that there are missing type hints for libraries, even if they should normally exist. This is really subtle; it's not at all obvious to an end user that --mypy-interpreter-constraints must match their requirements's compatibility. And that would be really error prone to try to hardcode it to match, imo.

On the other hand, idk if it's useful to expose this functionality for some reason, that you really don't want Pants to autoconfigure?

What do you all think?

cosmicexplorer

AWESOME!!!

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

src/python/pants/backend/python/subsystems/python_tool_base.py

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

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

# 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

🚢 🚢 🚢 🚢

coveralls · 2020-09-22T05:35:56Z

Coverage remained the same at 0.0% when pulling a3b7637 on Eric-Arellano:fix-mypy-py2 into 9aec06f on pantsbuild:master.

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

stuhood

Looks good... one maybe-substantive question though.

stuhood · 2020-09-22T19:17:46Z

src/python/pants/backend/python/subsystems/python_tool_base.py

-    default_interpreter_constraints: Sequence[str] = []
+    default_extra_requirements: ClassVar[Sequence[str]] = []
+    default_interpreter_constraints: ClassVar[Sequence[str]] = []
+    register_interpreter_constraints: ClassVar[bool] = True


Should this default to False? It seems like defaulting to limiting the total number of interpreters that are in use by only giving tools that require the ability to use an interpreter other than the one the code is using would be good.

stuhood · 2020-09-22T19:19:04Z

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

+    of running the target. For more information about PEX files, see https://www.pantsbuild.org/docs
+    /pex-files.


Won't this edit insert a space in the url?

Yeah, but I have no idea how to get docformatter to not automatically "fix" it :/ And I don't know why docformatter is suddenly making the change.

I can play with it more.

stuhood · 2020-09-22T19:23:25Z

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

+    # If the constraints are different for the tool than for the requirements, we must tell Pex to
+    # ignore errors. Otherwise, we risk runtime errors about missing dependencies.
+    if code_interpreter_constraints != tool_interpreter_constraints:
+        extra_env["PEX_IGNORE_ERRORS"] = "true"


Is this because we are adding the code to mypy's python path? Should we be passing it via MYPYPATH/mypy_path instead? https://mypy.readthedocs.io/en/stable/running_mypy.html#unable-to-find-module

If so, this relates to a pattern in the JVM: isolating "the code that is running" from "the code that is being operated on" as much as possible is good, because it avoids the need to "shade". While it sounds like mypy must run using the interpreter that it is checking for, isolating its dependencies from the code it is checking is still good.

That could work..definitely worth trying out. Thanks for the suggestion.

This works! If we set MYPYPATH to the wheel embedded in the Pex (or saved in the global cache), MyPy will use it. Even if the wheel is not compatible with the interpreter it's run with!

This test loads up a py2-only wheel:

MYPYPATH=~/.pex/installed_wheels/3780598d96101a8267782a69e0b66dc1a54f1def/x690-0.2.0-py2-none-any.whl mypy app.py

So, now my question is what is the safest and least hacky way for us to get the absolute paths to the requirments.pex's distributions? Keeping in mind complexities like remote execution, and that the chroot could change every run.

Extract from the requirements.pex itself by looking at its .deps folder. I don't know how to safely absolutify this, and maybe we don't need absolute paths as MyPy can use relative. I think we would run unzip on it, and then use Snapshot to get the file names. Feed the Digest into the runner Pex, and set MYPYPATH to the relative files?

Use the global cache by inspecting the --named-cache-dirs option. This sounds much less safe, e.g. issues with remoting.

#9206 is the ticket tracking the general case for shading. I had started working on a solution to the above which used a thin runner script to download requirements using the pex library API, and to execute that within a Process. I still think that could be a good solution for mypy instead of relying on pex internals. If the .deps folder is considered stable enough, I don't see a problem with unzipping it.

I think we will need this in the followup PR. We can delete it there if we don't need it. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano · 2020-09-23T00:53:03Z

Going to land this so that it makes it in the beta release today, as it at least unbreaks Python 2.

Will work on the improvement that uses MYPYPATH in a followup. Thanks for the help on this!

### Problem The wheels used by third-party code might not use the same constraints as what the MyPy Pex uses. This was causing errors (#10819), which we fixed in #10820. However, that fix was not perfect. Even if there were valid typed Python 2 wheels, we would ignore them. WIth Python 3, we had to be careful that `mypy.pex` used the exact same constraints as `requirements.pex`, so that, for example, we don't run MyPy with Py36 but resolve requirements with Py37. ### Solution Stop using `--pex-path` to join the `requirements.pex`, which has the effect of the wheels being put on `PYTHONPATH`. (We were then teaching MyPy to read from this PYTHONPATH through a custom launcher script.) Instead, we extract the wheel folders from `requirements.pex`, and teach our custom launcher script to look in these extracted `.deps/` folders through a new `EXTRACTED_WHEELS` env var. MyPy will use this to resolve the requirements, without us actually including the wheels in the final PEX or modifying PYTHONPATH. ### Result Our Python 2 test now fully works, even though one of the wheels is not compatible with Python 3. We no longer need to align `mypy.pex` with `requirements.pex`. #### Caveat: MyPy plugins must be installed with `--mypy-extra-requirements` An initial implementation did not use `EXTRACTED_WHEELS` and instead loaded the wheels via `PYTHONPATH`. This allowed for you to specify MyPy plugins via normal dependencies on `python_requirement_library` targets, because that requirement would get loaded via PYTHONPATH. Now, you cannot do this; you must specify the requirement via `--mypy-extra-requirements`. This caveat seems acceptable. For Pytest, it's convenient that we allow you to load plugins via normal requirements, as it allows you to only sometimes use the plugin. MyPy is much more uniform, however; MyPy runs in a single batch over the entire transitive closure, unlike Pytest running one test file at-a-time. Likewise, without Pants, it's hard to imagine how you could only sometimes use a MyPy plugin, given that MyPy only works with a single config file.

Eric-Arellano mentioned this pull request Sep 19, 2020

MyPy third-party requirements support is broken for Python 2 #10819

Closed

Eric-Arellano force-pushed the fix-mypy-py2 branch from 41733d6 to 925705e Compare September 19, 2020 05:08

Eric-Arellano added 3 commits September 21, 2020 20:43

WIP: Fix MyPy with Python 2-only third-party requirements

98e1c8b

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

Fix Pylint to ensure that it never has the runtime wheel issue

8b8f2ee

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

Fix MyPy to use the same constraints for the tool as for the requirem…

67c3b59

…ents # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano force-pushed the fix-mypy-py2 branch from 925705e to 67c3b59 Compare September 22, 2020 04:12

Eric-Arellano changed the title ~~WIP: Fix MyPy with Python 2-only third-party requirements~~ Fix MyPy with Python 2-only third-party requirements Sep 22, 2020

Eric-Arellano marked this pull request as ready for review September 22, 2020 04:17

Eric-Arellano requested review from benjyw, stuhood and cosmicexplorer September 22, 2020 04:17

Eric-Arellano commented Sep 22, 2020

View reviewed changes

cosmicexplorer reviewed Sep 22, 2020

View reviewed changes

Review feedback + restore code used by Black

cd3d1c1

# 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 22, 2020

View reviewed changes

Review feedback + restore code used by Black

bcaf54f

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

stuhood approved these changes Sep 22, 2020

View reviewed changes

Eric-Arellano mentioned this pull request Sep 22, 2020

Deprecate --interpreter-constraints option where it no-ops #10833

Merged

Eric-Arellano added 2 commits September 22, 2020 17:39

Merge branch 'master' of github.com:pantsbuild/pants into fix-mypy-py2

02d353d

Restore requires_python3{6,7}

a3b7637

I think we will need this in the followup PR. We can delete it there if we don't need it. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano merged commit f8a0b0f into pantsbuild:master Sep 23, 2020

Eric-Arellano deleted the fix-mypy-py2 branch September 23, 2020 01:48

Eric-Arellano mentioned this pull request Sep 24, 2020

Improve support of MyPy requirements when Python 2 used #10853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Eric-Arellano commented Sep 19, 2020 •

edited

Loading

Eric-Arellano Sep 22, 2020

Eric-Arellano Sep 22, 2020

cosmicexplorer Sep 22, 2020

Eric-Arellano Sep 22, 2020

Eric-Arellano Sep 22, 2020

cosmicexplorer left a comment

cosmicexplorer left a comment

coveralls commented Sep 22, 2020 •

edited

Loading

stuhood left a comment

stuhood Sep 22, 2020

stuhood Sep 22, 2020

Eric-Arellano Sep 22, 2020 •

edited

Loading

stuhood Sep 22, 2020

Eric-Arellano Sep 22, 2020

Eric-Arellano Sep 22, 2020

cosmicexplorer Sep 22, 2020

Eric-Arellano commented Sep 23, 2020

		of running the target. For more information about PEX files, see https://www.pantsbuild.org/docs
		/pex-files.

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

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

Conversation

Eric-Arellano commented Sep 19, 2020 • 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

cosmicexplorer left a comment

Choose a reason for hiding this comment

cosmicexplorer left a comment

Choose a reason for hiding this comment

coveralls commented Sep 22, 2020 • edited Loading

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano Sep 22, 2020 • 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

Eric-Arellano commented Sep 23, 2020

Eric-Arellano commented Sep 19, 2020 •

edited

Loading

coveralls commented Sep 22, 2020 •

edited

Loading

Eric-Arellano Sep 22, 2020 •

edited

Loading