Improve support of MyPy requirements when Python 2 used #10853
Conversation
# 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]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Eric-Arellano
requested review from
stuhood,
jsirois,
benjyw and
cosmicexplorer
…via `--mypy-extra-requirements` [ci skip-build-wheels] # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
Eric-Arellano
force-pushed
the
fix-mypy-py2-round2
branch
from
b11d915
to
d28aceb
Compare
| if not mypy.options.is_default("interpreter_constraints"): | ||
| tool_interpreter_constraints = mypy.interpreter_constraints | ||
| elif code_interpreter_constraints.requires_python38_or_newer(): | ||
| tool_interpreter_constraints = ("CPython>=3.8",) | ||
| elif code_interpreter_constraints.requires_python37_or_newer(): | ||
| tool_interpreter_constraints = ("CPython>=3.7",) | ||
| elif code_interpreter_constraints.requires_python36_or_newer(): | ||
| tool_interpreter_constraints = ("CPython>=3.6",) | ||
| else: | ||
| tool_interpreter_constraints = mypy.interpreter_constraints |
There was a problem hiding this comment.
This will get simplified soon with batching by interpreter constraints, because we will start to set MyPy's python_version ourselves.
For now, we have it this way to
a) ensure Py38 code uses Py38 to run MyPy, which is necessary to work properly, and
b) you don't have to specify python_version in mypy.ini because Pants will run with the correct Python interpreter, which is used to set the default
See #10750 for more about this.
| PYTHONPATH = frozenset( | ||
| os.path.realpath(p) | ||
| for p in os.environ.get('PYTHONPATH', '').split(os.pathsep) | ||
| ) |
There was a problem hiding this comment.
I removed this part because I don't think it's relevant in a v2 world with hermetic environments. Users can't set PYTHONPATH manually anymore. I think this was leftover from the original v1 script.
Lmk though if this needs to be added back.
| else code_interpreter_constraints | ||
| ) | ||
|
|
||
| if not mypy.options.is_default("interpreter_constraints"): |
There was a problem hiding this comment.
Is this block mypy-specific? Or could it be factored into a method on PythonToolBase?
There was a problem hiding this comment.
Yes, very MyPy specific. And also going to change super soon to instead be:
tool_interpreter_constraints = (
("CPython>=3.8",)
if mypy.options.is_default("interpreter_constraints") and code_interpreter_constraints.requires_python38_or_newer()
else mypy.interpreter_constraints
)(This is possible because we're going to set python_version automatically, so we no longer care about what interpreter is used to run MyPy. We only care that if there's Py38, we use Py38 because otherwise MyPy can't parse the AST due to typed-ast not working with Py38.)
| async def extract_distributions(pex: Pex, unzip_binary: UnzipBinary) -> ExtractedPexDistributions: | ||
| # We only unzip the `.deps` folder to avoid unnecessary work. Note that this will cause the | ||
| # process to fail if there is no `.deps` folder, so we need to use `FallibleProcessResult`. | ||
| argv = (*unzip_binary.extract_argv(archive_path=pex.name, output_dir="."), ".deps/*") |
There was a problem hiding this comment.
This is honestly so unreasonably slick.
[ci skip-build-wheels] # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
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.pexused the exact same constraints asrequirements.pex, so that, for example, we don't run MyPy with Py36 but resolve requirements with Py37.Solution
Stop using
--pex-pathto join therequirements.pex, which has the effect of the wheels being put onPYTHONPATH. (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 newEXTRACTED_WHEELSenv 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.pexwithrequirements.pex.Caveat: MyPy plugins must be installed with
--mypy-extra-requirementsAn initial implementation did not use
EXTRACTED_WHEELSand instead loaded the wheels viaPYTHONPATH. This allowed for you to specify MyPy plugins via normal dependencies onpython_requirement_librarytargets, 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.