Control PATH and PEX_PYTHON_PATH seperately. #10489
Conversation
Previously we used PATH to steer interpreter selection for hermetic PEX bootstrap and runtime. This led to fragile or impossible to solve setups. Switch to using PEX_PYTHON_PATH to both discover a bootstrap interpreter and steer runtime interpreter selection. Introduce a find_binary rule to facilitate this that uses ~portable bash for now. Also introduce a PexRuntimeEnvironment to steer bootstrap interpreter probing and allow specification of the PATH that should be exposed to the PEX runtime environment. Fixes pantsbuild#9760 # 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]
|
Hrm, remote PATH issues. I've unfortunately got to run. I'm hoping mauybe you can take this home @stuhood. I'll be checking in at gas stops on the big drive today. Sorry to not get this over the finish line. |
Not to worry! It sounds like @Eric-Arellano will be taking a look. Drive/be safe! |
…_search_path/dont_stomp_PATH # 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/download_pex_bin.py # src/python/pants/backend/python/rules/hermetic_pex.py # src/python/pants/backend/python/rules/pex.py # src/python/pants/backend/python/rules/pytest_runner.py # src/python/pants/backend/python/rules/run_setup_py.py # src/python/pants/backend/python/typecheck/mypy/rules.py # src/python/pants/engine/process.py [ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
…_search_path/dont_stomp_PATH [ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
…_search_path/dont_stomp_PATH
This will be useful in other contexts too.
| def register_options(cls, register): | ||
| super().register_options(register) | ||
| register( | ||
| "--binary-search-path", |
There was a problem hiding this comment.
Thoughts on an alternative name? Maybe simply --path? --binary-search-path makes me think this is meant to only be for finding the pex binary, which it's not about.
There was a problem hiding this comment.
I started at path but PEX_PATH is a thing already and would probably confuse a different set of users. I have no good ideas outside of where I settled here.
There was a problem hiding this comment.
Ah, true. I think --pex-path is pretty confusing. But, the counterpoint is that 95% of the time we expect this to be set in pants.toml, where it reads:
[pex]
path = []I don't find this as confusing with PEX_PATH. Thoughts?
There was a problem hiding this comment.
Expanding the help string might clarify the name as well: this is the PATH that a pex process will use, not just for itself, but also for anything it subprocesses, including pip, compilers, and etc.
There was a problem hiding this comment.
Thanks for the suggestions! I agree with both of you.
| ) | ||
|
|
||
| def level(self) -> Optional[LogLevel]: | ||
| return LogLevel.INFO if self.bootstrap_python else LogLevel.WARN |
There was a problem hiding this comment.
This should probably be at debug by default.
| def register_options(cls, register): | ||
| super().register_options(register) | ||
| register( | ||
| "--binary-search-path", |
There was a problem hiding this comment.
Expanding the help string might clarify the name as well: this is the PATH that a pex process will use, not just for itself, but also for anything it subprocesses, including pip, compilers, and etc.
| def iter_path_entries(): | ||
| for entry in self.options.binary_search_path: | ||
| if entry == "<PATH>": | ||
| # TODO(#9760): This is not very robust. We want to be able to read an env var |
There was a problem hiding this comment.
To be clear, rather than exposing an "env" intrinsic, I expect that the rest of #9760 (which I should probably break out) will deprecate the binary_search_path option, and move the PATH to being a global option that is only optionally overridden per Process. See the two comments at #9760 (comment) for more info.
| default=["python", "python3", "python2"], | ||
| metavar="<bootstrap-python-names>", | ||
| help=( | ||
| "The names of python binaries to search for to bootstrap PEX files with. Earlier " |
There was a problem hiding this comment.
An important thing to highlight here (assuming that it is true?) is that this has no influence on which interpreter ends up being used to actually run your code. The rest can probably just stay in docstrings (especially since we render a WARN currently if the lookup fails).
There was a problem hiding this comment.
Good suggestion. Thanks.
[ci skip-rust] [ci skip-build-wheels]
| if entry == "<PATH>": | ||
| path = os.environ.get("PATH") | ||
| if path: | ||
| for path_entry in os.pathsep.split(path): | ||
| for path_entry in path.split(os.pathsep): |
There was a problem hiding this comment.
Ouch. Thanks for figuring this out @Eric-Arellano.
|
Hello @jsirois I tried to use your work in I thought that I could finally fix it by specifying the following in my Which in my understanding means "Fetch python in PYENV but still forward $PATH to pex subprocesses so that they can use some tools" which fix the issue I had before with But I end up having the same issue as stated before that is a Thank you for your help! |
|
Hi @etiennedupont, thanks for trying it out, although bummer that it's still not quite working. What is the full stacktrace, including the Pants command? I'm curious if this is an issue with starting the Pex subprocess, vs. with your code that is being run by the subprocess.
This sounds very plausible. For example, with Pants's own integration tests, we have to set our Python interpreter paths both in |
|
Thank you for your answer @Eric-Arellano !
isn't there a way to supply a specific interpreter path to pex subprocesses? |
Previously we used PATH to steer interpreter selection for hermetic PEX
bootstrap and runtime. This led to fragile or impossible to solve
setups.
Switch to using PEX_PYTHON_PATH to both discover a bootstrap
interpreter and steer runtime interpreter selection. Introduce a
find_binary rule to facilitate this that uses ~portable bash for now.
Also introduce a PexRuntimeEnvironment to steer bootstrap interpreter
probing and allow specification of the PATH that should be exposed to
the PEX runtime environment.
Fixes #9760
[ci skip-rust]
[ci skip-build-wheels]