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

Lockfiles: implement --platform handling #12612

Closed
Eric-Arellano opened this issue Aug 20, 2021 · 13 comments
Closed

Lockfiles: implement --platform handling #12612

Eric-Arellano opened this issue Aug 20, 2021 · 13 comments

Comments

@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Aug 20, 2021

Background

Normally, lockfiles are intended to work on any platform, e.g. the same tool lockfile should usually work on macOS and Linux. If there are any platform-specific deps, that gets handled via environment markers in the lockfile: #12200

But when Pex's --platform is used, there are extra constraints:

  1. All deps must be available as prebuilt wheels compatible with the requested platforms.
  2. No other platforms should be allowed (Pants lockfile generation includes un-used dists and thus un-vetted dists. #12458)

Pex's lockfile generation will presumably implement this validation and tightening of the lockfile. But Pants still needs a way to model these platform-specific lockfiles.

Another key detail: support for multiple user lockfiles is coming with #11165. This is what will allow for "platform-aware" lockfiles at the same time as more generic "platform-agnostic" user lockfiles.

Proposed modeling

Lockfiles will now keep track of a platforms: list[str] metadata entry. If the lockfile is agnostic, it will be empty, else will include strings like macosx_11_0_arm64-cp-39-cp39.

As part of #11165, a user can set the pex_binary's field lockfile (resolve?) to any lockfile they like on disk. If the pex_binary has the platforms field set, we will validate that the lockfile is "platform-aware" and that the target's platforms are a subset of the lockfile's platforms. If not, we'll error and require either regenerating that lockfile or creating a new one.

Some implications:

  • A pex_binary with platforms must have a platform-aware lockfile (or no lockfile). Platform-agnostic lockfiles will not work.
  • Several pex_binarys can share the same platform-aware resolve/lockfile if they would like.
    • This satisfies a goal of the multiple lockfiles project: allow reuse of the same resolve, improving cache rates etc.
    • It's even valid for disjoint platforms, e.g. a macOS binary and Linux binary sharing the same lockfile. The lockfile will work with either macOS or Linux. The downside is that every dep used by both binaries must be compatible with both platforms.
  • There should be no need for "templating" - it was originally proposed to have a distinct lockfile per platform, in the format <name-of-resolve>-<platform>.txt. Instead, it's possible for a single lockfile to work with multiple platforms.
    • This means the user can name the lockfile whatever they'd like.
@jsirois
Copy link
Contributor

jsirois commented Aug 20, 2021

  1. No other platforms should be allowed

Its totally valid to say pex --interpreter-constraint ">=3.7,<3.10" --platform "foo" ... so the reason no other platform should be allowed is not #12458 but only because Pants doesn't let you mix ICs and --platform like that IIUC.

@Eric-Arellano
Copy link
Contributor Author

Its totally valid to say pex --interpreter-constraint ">=3.7,<3.10" --platform "foo" ...

Only if you are building locally, though, right, per pex-tool/pex#957? Which we don't support with Pants I think for simplicity more than anything, iirc.

Good point also that in the lockfile metadata, platforms would be mutually exclusive with interpreter_constraints.

--

Different note: see https://github.com/Eric-Arellano/pants-multiple-lockfiles-design/tree/main/disjoint-platform-binaries_distinct-lockfiles and https://github.com/Eric-Arellano/pants-multiple-lockfiles-design/tree/main/disjoint-platform-binaries_shared-lockfile for two examples of what platform-aware lockfiles look like.

I also rediscovered the earlier templating design from July https://github.com/Eric-Arellano/lockfile-platforms-problem. If this new scheme proves correct, I think it is much improved and will be easier to reason about.

@jsirois
Copy link
Contributor

jsirois commented Aug 26, 2021

Only if you are building locally, though, right, per pex-tool/pex#957? Which we don't support with Pants I think for simplicity more than anything, iirc.

I'm not sure what you mean by "only if building locally" - that's all Pex can ever do unless it grows a remoting capability. It (sometimes) builds wheels from sdists and that has to happen locally, it installs wheels in local chroots in ~/.pex/installed_wheels/... and it builds a PEX zip file - which is also a local operation.

I do think you're right on the simplicity bit. Pants could certainly support ~:

pex_binary(
    platforms=[...],
    interpreter_constraints="<no!>",
)

And take on the burden of the clunkieness and associated docs explaining it.

@Eric-Arellano
Copy link
Contributor Author

It (sometimes) builds wheels from sdists and that has to happen locally

I meant this: --resolve-local-platforms.

@jsirois
Copy link
Contributor

jsirois commented Aug 26, 2021

Aha. But no. It still is totally valid to say pex --interpreter-constraint ">=3.7,<3.10" --platform "foo" ... and not specify --seolve-local-platforms. That just means, for the --platform "foo", onluy accpet pre-built binary wheels - even if foo matches an interpreter on the local machine.

@jsirois
Copy link
Contributor

jsirois commented Aug 26, 2021

More broadly, I'm not aware of any combination of Pex CLI flags that's illegal or doesn't make sense that's not accompanied by Pex raising an error early that tells you so.

@Eric-Arellano
Copy link
Contributor Author

It still is totally valid to say pex --interpreter-constraint ">=3.7,<3.10" --platform "foo" ... and not specify --seolve-local-platforms. That just means, for the --platform "foo", onluy accpet pre-built binary wheels - even if foo matches an interpreter on the local machine.

While it's not an error to say that, do the interpreter constraints change any behavior if --resolve-local-platforms is not specified?

@jsirois
Copy link
Contributor

jsirois commented Aug 26, 2021

While it's not an error to say that, do the interpreter constraints change any behavior if --resolve-local-platforms is not specified?

No, they don't change behavior - but that terminology reveals a mistaken mental model I think. All the interpreter and platform selection options are additive. If you tell Pex --interpreter-constraints, at build-time it finds all local interpreters that match and resolves using each in parallel. If you also tack on one or more --platforms, it tack those on to the parallel resolve too. If you also tack on --resolve-local-platforms it tries - best effort - to turn any --platform you specified into an equivalent local interpreter. If it can, it uses that interpreter in the parallel resolve, if not, it uses the foreign platform resolution mechanism in Pip.

@jsirois
Copy link
Contributor

jsirois commented Aug 26, 2021

Not a perfect demonstration since --resolve-local-platforms doesn't change behavior when the dist happens to have a published wheel, but:

$ pex --interpreter-constraint "CPython==3.7.*" --intransitive pantsbuild.pants -osingle.pex
$ pex-tools single.pex info | jq .distributions
{
  "pantsbuild.pants-2.6.0-cp37-cp37m-manylinux2014_x86_64.whl": "bde9b095a8c53248444b586c09b5314064b54def"
}

$ pex --interpreter-constraint "CPython>=3.7,<3.9" --intransitive pantsbuild.pants -odouble.pex
$ pex-tools double.pex info | jq .distributions
{
  "pantsbuild.pants-2.6.0-cp37-cp37m-manylinux2014_x86_64.whl": "bde9b095a8c53248444b586c09b5314064b54def",
  "pantsbuild.pants-2.6.0-cp38-cp38-manylinux2014_x86_64.whl": "80cf54248a99c80d7a5c70a39fe6511e6ccdb6ff"
}

$ pex --interpreter-constraint "CPython>=3.7,<3.9" --platform macosx-11_0_arm64-cp-39-cp39 --platform linux-x86_64-cp-39-cp39 --intransitive pantsbuild.pants -oquadruple.pex
$ pex-tools quadruple.pex info | jq .distributions
{
  "pantsbuild.pants-2.6.0-cp37-cp37m-manylinux2014_x86_64.whl": "bde9b095a8c53248444b586c09b5314064b54def",
  "pantsbuild.pants-2.6.0-cp38-cp38-manylinux2014_x86_64.whl": "80cf54248a99c80d7a5c70a39fe6511e6ccdb6ff",
  "pantsbuild.pants-2.6.0-cp39-cp39-macosx_11_0_arm64.whl": "6d3dd3ec3d970f393d0d08759f39900ea4605af6",
  "pantsbuild.pants-2.6.0-cp39-cp39-manylinux2014_x86_64.whl": "11be57a5b40113f19301f1d16ff1b311de152cf2"
}

$ pex --interpreter-constraint "CPython>=3.7,<3.9" --platform macosx-11_0_arm64-cp-39-cp39 --platform linux-x86_64-cp-39-cp39 --resolve-local-platforms --intransitive pantsbuild.pants -oquadruple-rlp.pex
$ pex-tools quadruple-rlp.pex info | jq .distributions
{
  "pantsbuild.pants-2.6.0-cp37-cp37m-manylinux2014_x86_64.whl": "bde9b095a8c53248444b586c09b5314064b54def",
  "pantsbuild.pants-2.6.0-cp38-cp38-manylinux2014_x86_64.whl": "80cf54248a99c80d7a5c70a39fe6511e6ccdb6ff",
  "pantsbuild.pants-2.6.0-cp39-cp39-macosx_11_0_arm64.whl": "6d3dd3ec3d970f393d0d08759f39900ea4605af6",
  "pantsbuild.pants-2.6.0-cp39-cp39-manylinux2014_x86_64.whl": "11be57a5b40113f19301f1d16ff1b311de152cf2"
}

@Eric-Arellano
Copy link
Contributor Author

Eric-Arellano commented Jan 27, 2022

Coming back to this...I originally proposed that a pex_binary with platforms set would require a platform-aware resolve/lockfile. Now that we know how resolves are modeled, summarized in #13621 (comment), that would be horribly verbose to have to have a dedicated resolve/lockfile that is platform-aware. You would need to update all transitive deps, first-party and third-party, to claim compatibility with both the platform-agnostic & platform-aware resolves via compatible_resolves field. It seems likely those resolves would be composed of the same python_requirement targets, so the duplication would look fishy/unnecessary to users.

Instead, I think it should be sufficient to have a single "universal"-style PEX lock, which will include all bdist wheels. When you have a pex_binary with platforms, it will error at build-time if the lock is not compatible with the platforms. This is the same iiuc as the status quo with constraints file + platforms.

@jsirois does this sound right? Ack that "universal"-style has downsides.

--

Ah! Another approach is that we could still call it the same "resolve", which is just a logical name for the lock. Generate >1 lockfile for that resolve, where some are platform-aware. This would allow you to create more minimal locks without unvetted dists.

That would require a bit of design work, such as:

I think we could probably punt on implementing this until we get feedback it's a priority for users. But it would be good to confirm this approach is indeed feasible.

@jsirois
Copy link
Contributor

jsirois commented Jan 27, 2022

Instead, I think it should be sufficient to have a single "universal"-style PEX lock, which will include all bdist wheels. When you have a pex_binary with platforms, it will error at build-time if the lock is not compatible with the platforms. This is the same iiuc as the status quo with constraints file + platforms.

I think that's right. The only new wrinkle is the misplaced confidence; i.e.: you have a new step that is seperable - "generate lockfile and check it in" that succeeds and if CI doesn't exercise the lockfile against all consumers, you don't encounter any failures until later when the pex_binary with platforms is built.

@jsirois
Copy link
Contributor

jsirois commented Jan 27, 2022

A feature Pex could support is allowing --platform when generating universal locks and including checks that all deps have at least 1 artifact with wheel tags matching the platform tag set as supported by https://github.com/pantsbuild/pex/blob/cc3859bdbf068d299053f3557a79db3d51864592/pex/platforms.py#L129-L346. That would get you the fail-fast and advance the status quo.

@Eric-Arellano
Copy link
Contributor Author

I think that's right.

Yay! That is excellent news, makes this project much simpler for Pants. Closing this ticket as answered.

The only new wrinkle is the misplaced confidence;

Ack. Although, that is already a wrinkle in Pants's status quo.

A feature Pex could support is allowing --platform when generating universal locks and including checks that all deps have at least 1 artifact with wheel tags matching the platform tag set as supported by

That would be so cool. I opened pex-tool/pex#1595. Thanks for the idea!

Imo, this is not very urgent - it's useful and neat, but there's an acceptable workaround from Pants's perspective to run ./pants package :: in CI (which our example-python repo does)

Repository owner moved this from Todo to Done in Python multiple user lockfiles Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

2 participants