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

PexProcess and VenvPexProcess do not use append_only_caches. #11753

Closed
jsirois opened this issue Mar 20, 2021 · 1 comment · Fixed by #11760
Closed

PexProcess and VenvPexProcess do not use append_only_caches. #11753

jsirois opened this issue Mar 20, 2021 · 1 comment · Fixed by #11760
Assignees
Labels

Comments

@jsirois
Copy link
Contributor

jsirois commented Mar 20, 2021

Right now they avoid the plumbing needed for this by using an absolute named_caches based path for the PEX_ROOT. See comments here: #11040 (comment)

return PexEnvironment(
path=pex_runtime_env.path(pex_relevant_environment),
interpreter_search_paths=tuple(
python_setup.interpreter_search_paths(pex_relevant_environment)
),
subprocess_environment_dict=subprocess_env_vars.vars,
# TODO: This path normalization is duplicated with `engine_initializer.py`. How can we do
# the normalization only once, via the options system?
named_caches_dir=Path(global_options.options.named_caches_dir).resolve().as_posix(),
bootstrap_python=first_python_binary(),
)

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=os.path.join(self.named_caches_dir, "pex_root"),

env = {
**pex_environment.environment_dict(python_configured=pex.python is not None),
**(request.extra_env or {}),
}
input_digest = (
await Get(Digest, MergeDigests((pex.digest, request.input_digest)))
if request.input_digest
else pex.digest
)
return Process(
argv,
description=request.description,
level=request.level,
input_digest=input_digest,
env=env,
output_files=request.output_files,
output_directories=request.output_directories,
timeout_seconds=request.timeout_seconds,
execution_slot_variable=request.execution_slot_variable,
cache_scope=request.cache_scope,
)

This breaks remote execution where a PEX might be built in a container with a different named caches dir absolute path than where it is run.

The VenvPexProcess setup will additionally need a fix for its venv_dir which is likewise absolute and should be relativized against the append_only_caches dir:

pex_request = request.pex_request
seeded_venv_request = dataclasses.replace(
pex_request, additional_args=pex_request.additional_args + ("--venv", "--seed")
)
result = await Get(BuildPexResult, PexRequest, seeded_venv_request)
# Pex --seed mode outputs the path of the PEX executable. In the --venv case this is the `pex`
# script in the venv root directory.
venv_dir = PurePath(result.result.stdout.decode().strip()).parent

@jsirois
Copy link
Contributor Author

jsirois commented Mar 20, 2021

As discovered in #11742.

@jsirois jsirois self-assigned this Mar 20, 2021
jsirois added a commit to jsirois/pants that referenced this issue Mar 21, 2021
Previously we cheated and exported PEX_ROOT as the absolute path of the
named caches dir + "pex_root". Since we obtained the absolute path from
the local machine, this cheat was unmasked by remote execution where
the absolute path of the named caches dir was different.

Now we use a relative PEX_ROOT that ties in with append_only_caches.
The VenvPex scripts are updated to use relative paths as well.

Fixes pantsbuild#11753

# 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 Mar 21, 2021
Previously we cheated and exported PEX_ROOT as the absolute path of the
named caches dir + "pex_root". Since we obtained the absolute path from
the local machine, this cheat was unmasked by remote execution where
the absolute path of the named caches dir was different.

Now we use a relative PEX_ROOT that ties in with append_only_caches.
The VenvPex scripts are updated to use relative paths as well.

Fixes pantsbuild#11753

# 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 Mar 21, 2021
Previously we cheated and exported PEX_ROOT as the absolute path of the
named caches dir + "pex_root". Since we obtained the absolute path from
the local machine, this cheat was unmasked by remote execution where
the absolute path of the named caches dir was different.

Now we use a relative PEX_ROOT that ties in with append_only_caches.
The VenvPex scripts are updated to use relative paths as well.

Fixes #11753
jsirois added a commit to jsirois/pants that referenced this issue Mar 21, 2021
Previously we cheated and exported PEX_ROOT as the absolute path of the
named caches dir + "pex_root". Since we obtained the absolute path from
the local machine, this cheat was unmasked by remote execution where
the absolute path of the named caches dir was different.

Now we use a relative PEX_ROOT that ties in with append_only_caches.
The VenvPex scripts are updated to use relative paths as well.

Fixes pantsbuild#11753

(cherry picked from commit 4210c6c)

[ci skip-rust]

[ci skip-build-wheels]
jsirois added a commit that referenced this issue Mar 21, 2021
Previously we cheated and exported PEX_ROOT as the absolute path of the
named caches dir + "pex_root". Since we obtained the absolute path from
the local machine, this cheat was unmasked by remote execution where
the absolute path of the named caches dir was different.

Now we use a relative PEX_ROOT that ties in with append_only_caches.
The VenvPex scripts are updated to use relative paths as well.

Fixes #11753
Fixes #11742

(cherry picked from commit 4210c6c)
___
And in support of the above:

Simplify Black file specification. (#11762)
    
Black supports passing an explicit list of files and we have that. This
clears the way for having the pex_root named cache symlinked into the
Black Process chroot without having Black trying to recursively format
everything in there.

(cherry picked from commit c927cee)
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