Skip to content

Commit

Permalink
Type stubs should be registered as normal requirements, not in `[mypy…
Browse files Browse the repository at this point in the history
…].extra_requirements` (#12597)

### Background

Before Pants 2.6, using third-party type stubs was awkward because [we did not yet have dependency inference](https://blog.pantsbuild.org/introducing-pants-2-6/). Instead, you either had to explicitly add the dependency or hijack `[mypy].extra_requirements`. The latter would mean installing the dep more times than it's actually needed.

To allow putting type stubs in `[mypy].extra_requirements`, we had to install those deps _both_ in the `mypy.pex` tool PEX, and in the `typechecked_venv.pex`, which is a `VenvPex` where we grab the Python interpreter to point MyPy's `--python-executable` at. This meant installing `[mypy].extra_requirements` twice. 

### Solution

Instead, `[mypy].extra_requirements` should only be used for what is needed to run MyPy itself, such as MyPy plugins. It should not include type stubs, which should be installed via normal dependencies like in `requirements.txt`. 

Clearing this up has the benefits of:

- bringing conceptual clarity
- not installing `[mypy].extra_requirements` twice
- facilitating adding a tool lockfile for MyPy

### Awkward bit: MyPy plugins w/ type stubs embedded

Some requirements like `django-stubs` both act as a MyPy plugin and include type stubs. In fact, `django-stubs` was the motivating case for the original implementation. 

Now, you will need to install `django-stubs` and its kin in both `[mypy].extra_requirements` (for the plugin part) and as a normal dependency (for the type stubs). (The duplication of requirement strings could be solved by #12449). This is awkward, but conceptually sound.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano authored Aug 18, 2021
1 parent 7d9d405 commit fe9a2e2
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 32 deletions.
48 changes: 18 additions & 30 deletions src/python/pants/backend/python/typecheck/mypy/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ class MyPyRequest(TypecheckRequest):

def generate_argv(
mypy: MyPy,
typechecked_venv_pex: VenvPex,
*,
venv_python: str,
file_list_path: str,
python_version: Optional[str],
) -> Tuple[str, ...]:
args = [f"--python-executable={typechecked_venv_pex.python.argv0}", *mypy.args]
args = [f"--python-executable={venv_python}", *mypy.args]
if mypy.config:
args.append(f"--config-file={mypy.config}")
if python_version:
Expand Down Expand Up @@ -138,6 +138,7 @@ async def mypy_typecheck_partition(
SourceFiles, SourceFilesRequest(tgt.get(PythonSources) for tgt in partition.root_targets)
)

# See `requirements_venv_pex` for how this will get wrapped in a `VenvPex`.
requirements_pex_get = Get(
Pex,
PexFromTargetsRequest,
Expand All @@ -148,18 +149,6 @@ async def mypy_typecheck_partition(
),
)

# TODO(John Sirois): Scope the extra requirements to the partition.
# Right now we just use a global set of extra requirements and these might not be compatible
# with all partitions. See: https://github.com/pantsbuild/pants/issues/11556
mypy_extra_requirements_pex_get = Get(
Pex,
PexRequest(
output_filename="mypy_extra_requirements.pex",
internal_only=True,
requirements=PexRequirements(mypy.extra_requirements),
interpreter_constraints=partition.interpreter_constraints,
),
)
mypy_pex_get = Get(
VenvPex,
PexRequest(
Expand All @@ -173,20 +162,12 @@ async def mypy_typecheck_partition(
),
)

(
plugin_sources,
closure_sources,
roots_sources,
mypy_pex,
requirements_pex,
mypy_extra_requirements_pex,
) = await MultiGet(
plugin_sources, closure_sources, roots_sources, mypy_pex, requirements_pex = await MultiGet(
plugin_sources_get,
closure_sources_get,
roots_sources_get,
mypy_pex_get,
requirements_pex_get,
mypy_extra_requirements_pex_get,
)

python_files = determine_python_files(roots_sources.snapshot.files)
Expand All @@ -196,18 +177,25 @@ async def mypy_typecheck_partition(
CreateDigest([FileContent(file_list_path, "\n".join(python_files).encode())]),
)

typechecked_venv_pex_request = Get(
# This creates a venv with all the 3rd-party requirements used by the code. We tell MyPy to
# use this venv by setting `--python-executable`. Note that this Python interpreter is
# different than what we run MyPy with.
#
# We could have directly asked the `PexFromTargetsRequest` to return a `VenvPex`, rather than
# `Pex`, but that would mean missing out on sharing a cache with other goals like `test` and
# `run`.
requirements_venv_pex_request = Get(
VenvPex,
PexRequest(
output_filename="typechecked_venv.pex",
output_filename="requirements_venv.pex",
internal_only=True,
pex_path=[requirements_pex, mypy_extra_requirements_pex],
pex_path=[requirements_pex],
interpreter_constraints=partition.interpreter_constraints,
),
)

typechecked_venv_pex, file_list_digest = await MultiGet(
typechecked_venv_pex_request, file_list_digest_request
requirements_venv_pex, file_list_digest = await MultiGet(
requirements_venv_pex_request, file_list_digest_request
)

merged_input_files = await Get(
Expand All @@ -217,7 +205,7 @@ async def mypy_typecheck_partition(
file_list_digest,
plugin_sources.source_files.snapshot.digest,
closure_sources.source_files.snapshot.digest,
typechecked_venv_pex.digest,
requirements_venv_pex.digest,
config_file.digest,
]
),
Expand All @@ -237,7 +225,7 @@ async def mypy_typecheck_partition(
mypy_pex,
argv=generate_argv(
mypy,
typechecked_venv_pex,
venv_python=requirements_venv_pex.python.argv0,
file_list_path=file_list_path,
python_version=config_file.python_version_to_autoset(
partition.interpreter_constraints, python_setup.interpreter_universe
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,18 @@ def test_thirdparty_dependency(rule_runner: RuleRunner) -> None:


def test_thirdparty_plugin(rule_runner: RuleRunner) -> None:
# NB: We install `django-stubs` both with `[mypy].extra_requirements` and a user requirement
# (`python_requirement_library`). This awkwardness is because its used both as a plugin and
# type stubs.
rule_runner.write_files(
{
"BUILD": "python_requirement_library(name='django', requirements=['Django==2.2.5'])",
"BUILD": dedent(
"""\
python_requirement_library(
name='django', requirements=['Django==2.2.5', 'django-stubs==1.8.0'],
)
"""
),
f"{PACKAGE}/settings.py": dedent(
"""\
from django.urls import URLPattern
Expand Down Expand Up @@ -255,7 +264,7 @@ def test_thirdparty_plugin(rule_runner: RuleRunner) -> None:
result = run_mypy(
rule_runner,
[tgt],
extra_args=["--mypy-extra-requirements=django-stubs==1.5.0", "--mypy-version=mypy==0.770"],
extra_args=["--mypy-extra-requirements=django-stubs==1.8.0", "--mypy-version=mypy==0.812"],
)
assert len(result) == 1
assert result[0].exit_code == 1
Expand Down

0 comments on commit fe9a2e2

Please sign in to comment.