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

Pex repository not created if [python].enable_resolves=true and the resolve does not use a PEX lockfile #14991

Closed
stuhood opened this issue Apr 4, 2022 · 9 comments · Fixed by #15014
Assignees
Labels
Milestone

Comments

@stuhood
Copy link
Member

stuhood commented Apr 4, 2022

A pex repository is not created when using the combination of settings from the description, which results in skipping the --pex-repository subsetting operation.

@stuhood stuhood added this to the 2.11.x milestone Apr 4, 2022
@stuhood stuhood self-assigned this Apr 4, 2022
@stuhood
Copy link
Member Author

stuhood commented Apr 4, 2022

I've spent a few hours investigating this, but the code is fairly hairy. Will try to get it resolved by EOD Tuesday.

@thejcannon
Copy link
Member

To be more specific here, this is only an issue if the user is using a non-PEX-lockfile right? I don't see a foo_resolver option in https://www.pantsbuild.org/v2.11/docs/reference-python

@thejcannon
Copy link
Member

So I'm not sure this is an issue. There's only 2 reasons we need the repository.pex:

  1. [python].resolve_all_constraints is True
    • "Requires [python].requirement_constraints to be set." which is "Mutually exclusive with [python].enable_resolves."
    • So this doesn't apply
  2. [python]run_against_entire_lockfile is True
    • I don't see this in the description, and it default to False

I don't see a reason why enable_resolves needs to be (transitively) mutually exclusive with resolve_all_constraints though. That would "fix" this issue.

@Eric-Arellano thoughts?

@stuhood
Copy link
Member Author

stuhood commented Apr 4, 2022

To be more specific here, this is only an issue if the user is using a non-PEX-lockfile right? I don't see a foo_resolver option in https://www.pantsbuild.org/v2.11/docs/reference-python

Correct. Sorry: I meant lockfile_generator.

So I'm not sure this is an issue. There's only 2 reasons we need the repository.pex:

@thejcannon : The --pex-repository option is an important optimization for any monolithic resolve which cannot subset a resolve the way that PEX lockfiles can. If you don't use it with enable_resolves and lockfile_generator=poetry, then the effect is that every single target re-runs the resolve from scratch, which will hit the network to check PIP for updates and etc (although it should hit the PIP cache for actual wheels). PEX's native subsetting is much, much more efficient than that.

The reason that resolve_all_constraints is mutually exclusive with multiple resolves is that in multiple resolves, resolve_all_constraints is the right strategy (...for a poetry/manual lockfile), not that it's the wrong one.

But most importantly: it causes repositories which are not using PEX lockfiles to fail to build (ours in particular), possibly because the PEX-repository resolution codepath is more lenient (?). So we'll need to fix it.

@stuhood stuhood changed the title Pex repository not created if [python].enable_resolves=true and ![python]...resolver=pex Pex repository not created if [python].enable_resolves=true and ![python]...lockfile_generator=pex Apr 4, 2022
@thejcannon
Copy link
Member

An important distinction here is that Pants can and will consume a PEX lockfile regardless of lockfile_generator's setting.

@thejcannon thejcannon changed the title Pex repository not created if [python].enable_resolves=true and ![python]...lockfile_generator=pex Pex repository not created if [python].enable_resolves=true and the resolve uses a PEX lockfile Apr 4, 2022
@thejcannon thejcannon changed the title Pex repository not created if [python].enable_resolves=true and the resolve uses a PEX lockfile Pex repository not created if [python].enable_resolves=true and the resolve does not use a PEX lockfile Apr 4, 2022
@Eric-Arellano
Copy link
Contributor

Reminder that [python].lockfile_generator is not sufficient to know if it's a pex lockfile or not. a lot of work into that so you can do things like have some locks be pex and some manually generated. we need to call our helper function to inspect the lock and see if its json

@stuhood
Copy link
Member Author

stuhood commented Apr 4, 2022

An important distinction here is that Pants can and will consume a PEX lockfile regardless of lockfile_generator's setting.

Yes, and that's part of why the code is so complicated, unfortunately. We're going to need to support both for a while, so I'm trying to push the detection further upfront without too much churn.

@thejcannon
Copy link
Member

The reason that resolve_all_constraints is mutually exclusive with multiple resolves is that in multiple resolves, resolve_all_constraints is the right strategy (...for a poetry/manual lockfile), not that it's the wrong one.

Ah so really one way to think of this is it being implictly true in this case. That's what I missed in #14944

@stuhood stuhood added the bug label Apr 4, 2022
@Eric-Arellano
Copy link
Contributor

i suspect pex_from_targets will need to inspect the lock and call our helper function to determine if repo.pex should be used. pex.py can hopefully stay unchanged from josh's pr

stuhood added a commit that referenced this issue Apr 6, 2022
…15014)

As reported in #14980 and #14991: 1) the default layout of non-internal PEXes changed to `PACKED`, and 2) the PEX-repository subsetting optimization was disabled for manual or poetry lockfiles.

This change fixes those issues, and adds a test of the `pex_from_targets` codepath which decides which resolution strategy to use. But it also fairly significantly refactors the `PexRequest` consumption code to improve typesafety. In particular:
* Replaces the `is_all_constraints_resolve` special case with explicitly specified `PexLayout`s for those `PexRequests` (which would have been harder at the time when the special case was originally added, since `PexRequest.layout` didn't exist yet).
* Moves lockfile header parsing and PEX-native-detection into a `@rule` which produces a `LoadedLockfile`.
* Moves "subsetting of a lockfile or a repository PEX" to the `PexRequirements.from_subset` field, which takes a `LoadedLockfile` and asserts (not worth introducing another type yet probably) that the lockfile is PEX-native.
* Move the requirements which used to be attached to the `Lockfile`/`LockfileContents` off onto an `PexRequest.requirements = EntireLockfile` case, to assert that they are optional, and only used for metadata validation (rather than subsetting).

Fixes #14980 and fixes #14991.
stuhood added a commit to stuhood/pants that referenced this issue Apr 7, 2022
…antsbuild#15014)

As reported in pantsbuild#14980 and pantsbuild#14991: 1) the default layout of non-internal PEXes changed to `PACKED`, and 2) the PEX-repository subsetting optimization was disabled for manual or poetry lockfiles.

This change fixes those issues, and adds a test of the `pex_from_targets` codepath which decides which resolution strategy to use. But it also fairly significantly refactors the `PexRequest` consumption code to improve typesafety. In particular:
* Replaces the `is_all_constraints_resolve` special case with explicitly specified `PexLayout`s for those `PexRequests` (which would have been harder at the time when the special case was originally added, since `PexRequest.layout` didn't exist yet).
* Moves lockfile header parsing and PEX-native-detection into a `@rule` which produces a `LoadedLockfile`.
* Moves "subsetting of a lockfile or a repository PEX" to the `PexRequirements.from_subset` field, which takes a `LoadedLockfile` and asserts (not worth introducing another type yet probably) that the lockfile is PEX-native.
* Move the requirements which used to be attached to the `Lockfile`/`LockfileContents` off onto an `PexRequest.requirements = EntireLockfile` case, to assert that they are optional, and only used for metadata validation (rather than subsetting).

Fixes pantsbuild#14980 and fixes pantsbuild#14991.
# 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]
stuhood added a commit that referenced this issue Apr 7, 2022
…cherrypick of #15014) (#15032)

As reported in #14980 and #14991: 1) the default layout of non-internal PEXes changed to `PACKED`, and 2) the PEX-repository subsetting optimization was disabled for manual or poetry lockfiles.

This change fixes those issues, and adds a test of the `pex_from_targets` codepath which decides which resolution strategy to use. But it also fairly significantly refactors the `PexRequest` consumption code to improve typesafety. In particular:
* Replaces the `is_all_constraints_resolve` special case with explicitly specified `PexLayout`s for those `PexRequests` (which would have been harder at the time when the special case was originally added, since `PexRequest.layout` didn't exist yet).
* Moves lockfile header parsing and PEX-native-detection into a `@rule` which produces a `LoadedLockfile`.
* Moves "subsetting of a lockfile or a repository PEX" to the `PexRequirements.from_subset` field, which takes a `LoadedLockfile` and asserts (not worth introducing another type yet probably) that the lockfile is PEX-native.
* Move the requirements which used to be attached to the `Lockfile`/`LockfileContents` off onto an `PexRequest.requirements = EntireLockfile` case, to assert that they are optional, and only used for metadata validation (rather than subsetting).

Fixes #14980 and fixes #14991.

[ci skip-rust]
[ci skip-build-wheels]
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.

3 participants