Speed up resolving requirements through --use-first-matching-interpreter Pex flag
#10442
Conversation
…eter` Pex flag # Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
|
The dozens of CI failures show that this isn't safe to land :/ It seems it's not deterministic the interpreter we use to build vs. the interpreter we use to run? |
Correct if the implementation of the flag over in Pex land does not also cause the interpreter constraints to be omitted from the produced PEX-INFO in favor of setting the hashbang appropriately or some other such means of restricting the runtime interpreter to the build time interpreter. And just checked over there - this is in fact the issue. The feature from Pex is partial at the moment. Instead of pushing that feature to completion over in Pex would you be receptive to instead ditching the Pex feature and supporting this over in Pants? In https://github.com/pantsbuild/pants/pull/9747/files#diff-15ee4bfd6c4feec32a8fd4f8fd2faa10R42 I already showed how to do this and that PR has other benefits besides. If yes, I can take that PR back up to replace this one. |
My instinct is to prefer us using Pex as a CLI tool, rather than as a library. I've found it really helpful when debugging with users that we output the exact Pex command we're running. It helps us to first get things behaving properly with Pex, and then figure out how to correct Pants. What are the benefits you mention if we use Pex as a library rather than binary? |
On closer inspection, the primary benefit in that PR aside from --single which you try to implement here and loose pexes (unzipped from the get-go which the Pex CLI does not officially support) was only relevant to recursive resolve (splitting download of distributions from building of wheels / PEXes). Since recursive resolves proved non-performant for the majority of use cases that's no longer a valid reason. There are benefits though and those are primarily generic to any tool we use outside the Pex tool. Namely, we need not warp the tool towards Pants ends, instead we can compose the tools in our own wrapper. In the case of Pex we have a special relationship (we own it), so its easy to warp but still sometimes questionable to do so - aka: does this feature make sense from the Pex user's perspective Pants aside? Sometimes not so much. Towards this Pants has two current needs that don't map well to Pex use cases; yet composing the APIs exposed could support cleanly:
I do agree with your instinct to be able to run tools Pants runs and iterate. I think though that it would be more useful to generally support all tools Pants uses - internal or external. I think this could be done with: Where we teach Process executions to output all the arguments following |
|
Re the above bit about generically debuggable process executions, given 1 simple new goal and 1 simple new rule you'd have:
$ git diff src/python/pants/backend/python/typecheck/mypy/rules.py
diff --git a/src/python/pants/backend/python/typecheck/mypy/rules.py b/src/python/pants/backend/python/typecheck/mypy/rules.py
index 7ea0e628e..8bffeab1e 100644
--- a/src/python/pants/backend/python/typecheck/mypy/rules.py
+++ b/src/python/pants/backend/python/typecheck/mypy/rules.py
@@ -20,6 +20,7 @@ from pants.backend.python.subsystems import python_native_code, subprocess_envir
from pants.backend.python.subsystems.subprocess_environment import SubprocessEncodingEnvironment
from pants.backend.python.target_types import PythonSources
from pants.backend.python.typecheck.mypy.subsystem import MyPy
+from pants.core.goals.execute_process import DebuggableProcess
from pants.core.goals.typecheck import TypecheckRequest, TypecheckResult, TypecheckResults
from pants.core.util_rules import determine_source_files, strip_source_roots
from pants.engine.addresses import Addresses
@@ -121,7 +122,7 @@ async def mypy_lint(
env={"PEX_EXTRA_SYS_PATH": ":".join(prepared_sources.source_roots)},
description=f"Run MyPy on {pluralize(len(prepared_sources.snapshot.files), 'file')}.",
)
- result = await Get(FallibleProcessResult, Process, process)
+ result = await Get(FallibleProcessResult, DebuggableProcess(process))
return TypecheckResults(
[TypecheckResult.from_fallible_process_result(result, typechecker_name="MyPy")]
)
And the suggested repro works **: ** There is a technicality here where the @__files.txt doesn't pass through correctly since Pants options parsing - apparently buggily - eagerly sees the @__files.txt and tries to Fromfile the passthru arg. As such for the example run I left off that last arg. Does that general thrust alleviate your concern? |
|
Bummer, this still isn't working properly. Possibly due to platforms? |
|
Sounds right. But saying first plus adding platforms doesn't make sense. Can the platforms just always be stripped for first requests? |
|
Same for first + multiple --python args. The feature is super fragile and you can really only use interpreter constraints together with first usefully. |
…ing-interp # Conflicts: # src/python/pants/backend/awslambda/python/awslambda_python_rules.py # src/python/pants/backend/python/lint/bandit/rules.py # src/python/pants/backend/python/lint/black/rules.py # src/python/pants/backend/python/lint/docformatter/rules.py # src/python/pants/backend/python/lint/flake8/rules.py # src/python/pants/backend/python/lint/isort/rules.py # src/python/pants/backend/python/lint/pylint/rules.py # src/python/pants/backend/python/rules/coverage.py # src/python/pants/backend/python/rules/repl.py # src/python/pants/backend/python/rules/run_setup_py.py # src/python/pants/backend/python/typecheck/mypy/rules.py
…ing-interp # Conflicts: # src/python/pants/backend/python/rules/download_pex_bin.py # src/python/pants/backend/python/rules/pex.py
John pointed out that this is necessary for the runtime to be safe.
# 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]
…ing-interp [ci skip-rust] [ci skip-build-wheels]
|
@Eric-Arellano this should finally be ready to go. Sorry that took so long to figure out! |
…ing-interp # Conflicts: # src/python/pants/backend/python/rules/pytest_runner.py # src/python/pants/backend/python/rules/repl.py # src/python/pants/backend/python/rules/run_python_binary.py
By default, when you pass a loose interpreter constraint to Pex like
>=3.6, Pex will resolve for every interpreter discovered, e.g. 3.6 and 3.7 and 3.8. This results in the Pex being compatible with more interpreters at runtime.However, the vast majority of our Pexes are never actually distributed to end-users so we don't care about compatibility - we only need one interpreter to work. The only time we need that compatibility is when using
awslambdaandbinary, because those Pexes get materialized and distributed to the end-user.Closes #10380 and #8685.
Benchmark
Running
./pants --no-process-execution-use-local-cache --no-dynamic-ui --no-pantsd lint src/python/pants/util/strutil.py, which will force building 4 Pex files, and output the starting and completion times.[ci skip-rust-tests]