-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
pants 2.5 dependency issues with cross-platform targets #12222
Comments
Because this repo sets A hitch in this hypothesis though is that |
There is no hitch. There is no wheel: https://pypi.org/project/gast/0.2.2/#files Foreign resolves (platforms are used here instead of ICs) must have all dists available as pre-built wheels. Pex has an option to try to turn platforms into local interpreters so it can build sdists when that's all that's available, but Pants doesn't use that option or plumb it. |
So the bug here is a "lock file" really only works for a single interpreter (or a single platform - aka foreign interpreter). I had pointed out Pants own lockfile is buggy. If you regenerate Pants constraints.txt with python 3.7 and python 3.9 you get different results. That much is correct! Pants internals though assume 1 "lockfile" works for many interpreters, that can be incorrect. And this bug shows we further don't consider the platform case. |
See #12200 |
Thanks @oliverdain! In the meantime to redesigning our lockfile support in 2.6 or 2.7, I think the best fix is to change our |
That sounds right. The bug is still present for IC ranges as layed out in #12200 after that fix, but this gets back status quo. |
Well, using constraints at all really isn't correct, since who knows what the constraints are for that Linux platform when I'm running on Mac, but I think that was status quo. |
Cool. Ack that the status quo is still broken. I'll pair with @wilsonliam today on fixing this and cherry-pick to 2.5. |
Thanks for the fix everyone!
Does this mean there's a patch release I can pull and start using? |
@wilsonliam is cherry-picking into 2.5 and 2.6 right now, and then I'll try to do a release this afternoon before logging off for July 4 weekend. Pardon the delay the past 2 weeks! |
…orms` (pantsbuild#12268) Closes pantsbuild#12222 and adds an associated test for this edge case. [ci skip-rust] [ci skip-build-wheels]
…orms` (pantsbuild#12268) Closes pantsbuild#12222 and adds an associated test for this edge case. [ci skip-rust] [ci skip-build-wheels]
This allows you to use the new lockfile format, generated by pip-tools via `./pants --tag=-lockfile_ignore lock ::` and #12300. A lockfile cannot be used at the same time as a constraints file. This makes the code easier to implement and means that we don't break any prior APIs. We will likely deprecate constraints when the dust settles. There are several major deficiencies: - Only `pex_from_targets.py` consumes this lockfile. This means that tool lockfiles will now have no constraints and no lockfile, for now. - Does not handle requirements disjoint to the lockfile. - Does not support multiple user lockfiles, which, for example, is necessary to properly handle `platforms` with `pex_binary` and `python_awslambda`: we need one lockfile per platform, as demonstrated in https://github.com/Eric-Arellano/lockfile-platforms-problem/tree/main/multiple_pex__stu_proposal. ### Lockfile vs. constraints file (and `[python-setup].resolve_all_constraints`) We're currently using pip's `--constraints` file support, which allows you to specify constraints that may not actually be used. At the same time, we default to `[python-setup].resolve_all_constraints`, which _does_ first install the entire constraints file, and then uses Pex's repository PEX feature to extract the relevant subset. This is generally a performance optimization, but there are some times `--resolve-all-constraints` is not desirable: 1. It is not safe to first install the superset, and you can only install the proper subset. This especially can happen when `platforms` are used. See #12222. - We proactively disable `--resolve-all-constraints` when `platforms` are used. 2. User does not like the performance tradeoff, e.g. because they have a huge repository PEX so it's slow to access. -- In contrast, this PR stops using `--constraints` and roughly always does `[python-setup].resolve_all_constraints` (we now run `pex -r requirements.txt --no-transitive` and use repository PEXes). Multiple user lockfiles will allow us to solve the above issues: > 1. It is not safe to first install the superset, and you can only install the proper subset. We'll have a distinct lockfile for each `platform`, which avoids this situation. See https://github.com/Eric-Arellano/lockfile-platforms-problem/tree/main/multiple_pex__stu_proposal for an example. > 2. User does not like the performance tradeoff They can use multiple lockfiles to work around this. -- Always using `[python-setup].resolve_all_constraints` reduces complexity: less code to support, fewer concepts for users to learn. Likewise, if we did still want to use `--constraints`, we would also need to upgrade Pex to use Pip 21+, which gained support for URL constraints. We [hacked around URL constraints before](#11907), but that isn't robust. However, Pip 21+ drops Python 2 and 3.5 support: we'd need to release Pex 3 w/o Py2 support, and upgrade Pants to have workarounds that allow Py2 to still be used. To avoid project creep, it's better to punt on Pex 3. [ci skip-rust] [ci skip-build-wheels]
…ild#15031) pantsbuild#14995 shows that pantsbuild#12222 re-broke for the `enable_resolves` case, since the fix for the issue was only checking the `resolve_all_constraints` field. Fixes pantsbuild#14995. # 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]
I have a pants build with one target that tries to build a cross-platform pex file:
That built fine on pants 2.4 (and still does) but started to fail when I upgraded to pants 2.5 with the following:
Relevant bits of
pants.toml
:Also note:
gast
only transitively via TensorFlow and this target does not depend on TensorFlow or gast so there's no need to resolve that dependencyThe text was updated successfully, but these errors were encountered: