Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interactive PEX execution leaks PEX_ROOT at $BUILD_ROOT/.cache/pex_root. #12055

Closed
jsirois opened this issue May 11, 2021 · 0 comments · Fixed by #12066
Closed

Interactive PEX execution leaks PEX_ROOT at $BUILD_ROOT/.cache/pex_root. #12055

jsirois opened this issue May 11, 2021 · 0 comments · Fixed by #12066
Assignees
Labels

Comments

@jsirois
Copy link
Contributor

jsirois commented May 11, 2021

For example:

$ rm -rf .cache
$ ./pants run testprojects/src/python/hello/main/main.py 
09:27:23.34 [INFO] Completed: Building main.pex
09:27:24.12 [INFO] Completed: Building requirements.pex with 1 requirement: ansicolors==1.1.8
Hello, world!
$ tree .cache/
.cache/
└── pex_root
    ├── code
    │   └── da39a3ee5e6b4b0d3255bfef95601890afd80709
    │       ├── __main__.py
    │       └── PEX-INFO
    └── installed_wheels
        └── c7b5d77e89855f9b02c99e84cd863c5bdc2be329
            └── ansicolors-1.1.8-py2.py3-none-any.whl
                ├── ansicolors-1.1.8.dist-info
                │   ├── DESCRIPTION.rst
                │   ├── INSTALLER
                │   ├── METADATA
                │   ├── metadata.json
                │   ├── RECORD
                │   ├── REQUESTED
                │   ├── top_level.txt
                │   └── WHEEL
                └── colors
                    ├── colors.py
                    ├── csscolors.py
                    ├── __init__.py
                    ├── __pycache__
                    │   ├── colors.cpython-37.pyc
                    │   ├── colors.cpython-38.pyc
                    │   ├── csscolors.cpython-37.pyc
                    │   ├── csscolors.cpython-38.pyc
                    │   ├── __init__.cpython-37.pyc
                    │   ├── __init__.cpython-38.pyc
                    │   ├── version.cpython-37.pyc
                    │   └── version.cpython-38.pyc
                    └── version.py

9 directories, 22 files

The issue here is that PexEnvironment assumes it's only used by a Process:

def environment_dict(self, *, python_configured: bool) -> Mapping[str, str]:
"""The environment to use for running anything with PEX.
If the Process is run with a pre-selected Python interpreter, set `python_configured=True`
to avoid PEX from trying to find a new interpreter.
"""
d = dict(
PATH=create_path_env_var(self.path),
PEX_INHERIT_PATH="false",
PEX_IGNORE_RCFILES="true",
PEX_ROOT=str(self.pex_root),
**self.subprocess_environment_dict,
)
# NB: We only set `PEX_PYTHON_PATH` if the Python interpreter has not already been
# pre-selected by Pants. Otherwise, Pex would inadvertently try to find another interpreter
# when running PEXes. (Creating a PEX will ignore this env var in favor of `--python-path`.)
if not python_configured:
d["PEX_PYTHON_PATH"] = create_path_env_var(self.interpreter_search_paths)
return d
@property
def pex_root(self) -> PurePath:
return PurePath(".cache/pex_root")

In the InteractiveProcess case when run_in_workspace = True (the case for the run and repl goals) the assumptions of PexEnvironment are wrong.

@jsirois jsirois self-assigned this May 11, 2021
jsirois added a commit to jsirois/pants that referenced this issue May 12, 2021
Previously we'd leak `.cache/pex_root` to the workspace. Beyond making it
easier to (mistakenly) corrupt the Pex cache for interactive Pex processes
it also could degrade performance of those processes since this was not
a symlink to the `pex_root` named cache used by sandboxed processes.

Force user's of PexEnvironment methods that support setting up a process
to declare what type of process they are setting up by moving those
methods to CompletePexEnvironment with one concrete implementation for
sandboxed processes and one for workspace processes.

Fixes pantsbuild#12055

[ci skip-rust]
[ci skip-build-wheels]
jsirois added a commit that referenced this issue May 12, 2021
Previously we'd leak `.cache/pex_root` to the workspace. Beyond making it
easier to (mistakenly) corrupt the Pex cache for interactive Pex processes
it also could degrade performance of those processes since this was not
a symlink to the `pex_root` named cache used by sandboxed processes.

Force user's of PexEnvironment methods that support setting up a process
to declare what type of process they are setting up by moving those
methods to CompletePexEnvironment with one concrete implementation for
sandboxed processes and one for workspace processes.

Fixes #12055
jsirois added a commit to jsirois/pants that referenced this issue May 12, 2021
Previously we'd leak `.cache/pex_root` to the workspace. Beyond making it
easier to (mistakenly) corrupt the Pex cache for interactive Pex processes
it also could degrade performance of those processes since this was not
a symlink to the `pex_root` named cache used by sandboxed processes.

Force user's of PexEnvironment methods that support setting up a process
to declare what type of process they are setting up by moving those
methods to CompletePexEnvironment with one concrete implementation for
sandboxed processes and one for workspace processes.

Fixes pantsbuild#12055

(cherry picked from commit 0a01eaf)

# 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]
jsirois added a commit to jsirois/pants that referenced this issue May 12, 2021
Previously we'd leak `.cache/pex_root` to the workspace. Beyond making it
easier to (mistakenly) corrupt the Pex cache for interactive Pex processes
it also could degrade performance of those processes since this was not
a symlink to the `pex_root` named cache used by sandboxed processes.

Force user's of PexEnvironment methods that support setting up a process
to declare what type of process they are setting up by moving those
methods to CompletePexEnvironment with one concrete implementation for
sandboxed processes and one for workspace processes.

Fixes pantsbuild#12055

(cherry picked from commit 0a01eaf)

# 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]
jsirois added a commit that referenced this issue May 12, 2021
Previously we'd leak `.cache/pex_root` to the workspace. Beyond making it
easier to (mistakenly) corrupt the Pex cache for interactive Pex processes
it also could degrade performance of those processes since this was not
a symlink to the `pex_root` named cache used by sandboxed processes.

Force user's of PexEnvironment methods that support setting up a process
to declare what type of process they are setting up by moving those
methods to CompletePexEnvironment with one concrete implementation for
sandboxed processes and one for workspace processes.

Fixes #12055

(cherry picked from commit 0a01eaf)
thejcannon added a commit that referenced this issue Dec 5, 2022
This change makes it so we use a `VenvPex` to run Python sources, which is a speed boost (I measure a gain of about ~500ms, which is also quoted in `pex.py`).

In order to make this work (specifically ensuring we don't revert the fix for #12055) we now have to weave the complete pex environment through to `VenvPexRequest`.
thejcannon added a commit to thejcannon/pants that referenced this issue Dec 9, 2022
This change makes it so we use a `VenvPex` to run Python sources, which is a speed boost (I measure a gain of about ~500ms, which is also quoted in `pex.py`).

In order to make this work (specifically ensuring we don't revert the fix for pantsbuild#12055) we now have to weave the complete pex environment through to `VenvPexRequest`.
thejcannon added a commit that referenced this issue Dec 9, 2022
This change makes it so we use a `VenvPex` to run Python sources, which is a speed boost (I measure a gain of about ~500ms, which is also quoted in `pex.py`).

In order to make this work (specifically ensuring we don't revert the fix for #12055) we now have to weave the complete pex environment through to `VenvPexRequest`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant