`[python].resolves_to_constraints_file` works with non-Pex lockfiles and without lockfiles #16476

Eric-Arellano · 2022-08-11T03:33:51Z

Part of the per-resolve config project: https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit.

There is no need for us to artificially limit constraints files to PEX lockfiles. It is a useful feature when manually managing lockfiles, along with disabling lockfiles for tools.

A follow up will propose deprecating [python].requirement_constraints in favor of this new option.

[ci skip-rust]
[ci skip-build-wheels]

…on-Pex lockfiles # 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]

Eric-Arellano · 2022-08-11T03:36:27Z

src/python/pants/backend/python/util_rules/local_dists.py

+            # It is safe to not set the resolve_name because building this PEX will not make
+            # network calls, thanks to `--intransitive` and having the wheel sources present.
+            # TODO: is that actually true?
+            requirements=PexRequirements(wheels, resolve_name=None),


@benjyw and I DMed a little about this, but neither of us have rigorously verified this claim. This TODO blocks the PR, and I'll look into it tomorrow

Alright, I tested this out by adding install_requires to the setup() call in local_dists_test.py, then changing pex.py to only set --no-pypi for local_dists.pex. The test still succeeded, as @benjyw and I expected.

Did you test for an install_requires that is sdist only, but uses a PEP-517 / PEP-518 build backend? That should definitely need network access.

I did not. Either way, because this PR does not impact [python-repos] at all, it is now irrelevant. I removed the misleading comment.

Ok, great.

I looked more at that code and concur anyhow. Since the requirements are in fact all wheel file local paths, i.e.: locally built already, no network access should happen. It's only in the dists.py rules where that happens and indexes are potentially needed.

jsirois · 2022-08-11T13:30:54Z

We have two places that don't have lockfiles associated with them, and never will:

PEP-517 build backend when building a python_distribution. There may be any arbitrary build backend, and multiple of them, so we can't have a single lockfile.

You have way more confidence than I would. I'm not sure where you get that from since an individual python_distribution target is metadata about a local project using a single build system. That target could clearly - and probably desirably - point to lock file data for its [build-system] requires. So it may be a shortcoming of the code today that Pants doesn't let you lock a build system, but this is in no way an inherent problem - Pants clearly could support this, and eventually a PEP probably should. This, in fact, is why PEP-665 died in committee.

Perhaps this is just a problem with your 1st sentence there.

jsirois

It seems to me neither "no-user-resolve" nor "pep-517-build-backend" is needed at all.

jsirois · 2022-08-11T13:35:31Z

src/python/pants/backend/python/subsystems/setup.py

-            `[python].lockfile_generator = "pex"` and run the `generate-lockfiles` goal.
+            The constraints file will still be used if you are using a custom-managed lockfile
+            for a particular resolve, or if you disabled lockfiles for a certain tool. If you are
+            not using `[python].resolves` yet, use the key `{RESOLVE_OPTION_KEY__NO_USER_RESOLVE}`


I though the plan was to eventually mandate locks. If so, this is adding a feature that will go away, in which case why not just not add the feature. If you want to use constraints in the classic Pip use-case way you must use lock files.

I though the plan was to eventually mandate locks.

This has not been finalized. We require it for JVM, but have not committed to that for Python.

If so, this is adding a feature that will go away, in which case why not just not add the feature. If you want to use constraints in the classic Pip use-case way you must use lock files.

So, the more important options are [python].resolves_to_find_links and [python].resolves_to_indexes. In the design doc's deprecation plan, I propose migrating [python-repos] into those new [python] options, rather than keeping around the old and new, which IMO is confusing.

As explained there, I think it's desirable to let you change the options for no-user-resolve while letting other resolves have other options. My first design was that you must use __default__ to set no-user-resolve, until this new improved idea.

So, if we have no-user-resolve already for [python].resolves_to_find_links, it is trivial to also support constraints files.

Was there ever any feedback from the 1 user that said they'd like this feature ([python].resolves_to_find_links + [python].resolves_to_indexes)? IIRC they actually just said only having a global setting for each of these "was a pain".

That user did not reply.

I revised the design doc's motivation section:

In the past, we've had issues with [python].requirement_constraints applying to tool lockfiles too because sometimes users needed different pins for tools than user code, so we reverted that behavior. Generally, the multiple resolves feature is intended to give users flexibility, such as using two different versions of Django in the same repository; likewise, it's reasonable to need two different versions of constraints in a repo. This proposal builds off of that flexibility to also allow customizing settings like when to use a custom index. Most users won't need this flexibility, but we can expect some will. For example, a user today said:

"Our custom repo is somewhat problematic, so ability to namespace it and only reference it when needed would probably be a win."

I don't think the new words change the situation. I still see no actual reason a non-global configuration is needed for repos + indexes. It would be great to have a use case with no reasonable workaround before basing reasoning off it.

I don't want to strongarm us migrating from [python-repos] to [python].resolves_to_{indexes,find_links}, nor migrating [python].manylinux to be per-resolve. So I will revert the parts of this PR that are prework for those changes, and have this PR solely make sense in the context of per-resolve constraints files.

Then, we can evaluate whether to migrate each option individually.

I can also revert no-user-resolve and leave it for the [python].requirements_constraint deprecation, so it can be considered in one PR.

(Seems like there is little controversy that per-resolve constraints files make sense, given we allow the input requirements themselves to differ per resolve.)

jsirois · 2022-08-11T13:41:15Z

src/python/pants/backend/python/subsystems/setup.py

+            to set a constraints file for your user code (or set it via the key
+            `{RESOLVE_OPTION_KEY__DEFAULT}`).
+
+            If you are building PEP-517 pyproject.toml distributions via the `python_distribution`


This is only useful for python_distribution projects that do not publish an sdist. Anyone unfortunate enough to need to consume the sdist will not have access to the constraints fixes since there is no PEP support for this. All there is support for is [build-system] requires which gives you the tool needed, just add the constraint as a top-level requirement in the list.

Given that and the fact that, like tools, and build backend or tool that is actually used in the will get yelled at if gymnastics are needed to install / use their tool or build backend and the problem will be promptly fixed as a matter of course. You had pointed to black as a counter-example of this recently, but it's a perfect example. They've had 2 issues in the past year, each fixed in days because people yelled.

Thank you for taking the time to walk me through that. That makes sense.

I will remove this key from the constraints option. As explained above at #16476 (comment), I think we do need this resolve key for [python].resolves_to_find_links migration to work, right? But we do not need to wire up this "resolve" to a constraints file.

# 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]

Eric-Arellano · 2022-08-11T14:35:08Z

I'm not sure where you get that from since an individual python_distribution target is metadata about a local project using a single build system. That target could clearly - and probably desirably - point to lock file data for its [build-system] requires. So it may be a shortcoming of the code today that Pants doesn't let you lock a build system, but this is in no way an inherent problem - Pants clearly could support this, and eventually a PEP probably should. This, in fact, is why PEP-665 died in committee.

Thanks for explaining that! You're right. I revised the statement.

…ile-always-used [ci skip-build-wheels]

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

…-backend` # 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]

# 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]

Eric-Arellano · 2022-08-11T22:02:48Z

src/python/pants/backend/python/util_rules/pex_from_targets.py

                requirements = dataclasses.replace(
-                    requirements, constraints_strings=loaded_lockfile.constraints_strings
+                    requirements,
+                    constraints_strings=loaded_lockfile.constraints_strings,
                )


This means that this PR is not safe to land :( using a pex_binary target with platforms specified results in the error

This state is not legal because it would mean there's ambiguity with which constraints to use.

I need to understand pex_from_targets.py better before landing this.

benjyw · 2022-08-12T00:38:28Z

src/python/pants/backend/python/util_rules/dists.py

@@ -94,7 +94,7 @@ async def find_build_system(request: BuildSystemRequest, setuptools: Setuptools)
                raise InvalidBuildConfigError(
                    f"No requires found in the [build-system] table in {file_content.path}"
                )
-            ret = BuildSystem(PexRequirements(requires), build_backend)
+            ret = BuildSystem(PexRequirements(requires, resolve_name=None), build_backend)


We could/should support lockfile use for a buildsystem in the future, I guess.

Eric-Arellano · 2022-08-12T20:31:53Z

So after two days struggling with pex_from_targets.py and pex.py, I realize this PR does not make sense for manually managed lockfiles, along with poetry-generated-a-la-Pants. The lockfile is already generated: everything is already pinned! Constraints would only be useful when generating the lockfile in the first place, which we aren't going to hook up to the deprecated Poetry generation, and we cannot hook up to manual lockfile generation.

That leaves us with no lockfiles, such as [black].lockfile = none, and no-user-resolve key. Do we care about supporting this? no-user-resolve allows us to migrate [python].requirement_constraints to the new system, but we could simply deprecate that mechanism entirely in favor of resolves..Constraints file is a nice-to-have, and I think it's reasonable to only get that goody if you opt-into Pex lockfiles.

… more powerful `[python].resolves_to_only_binary` and `[python].resolves_to_no_binary` (#16513) Part of the per-resolve config project at https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit. We already with multiple resolves allow you to have conflicting versions of the same requirement, e.g. Django 2 vs Django 3. So, it's useful to also allow those resolves to set different options for `--no-binary` and `--only-binary`, as you might only need it for certain versions of a project or for certain contexts. This only works with Pex lockfiles, with similar reasoning to why we closed #16476. This adds the options to the lockfile header, making progress on #12832.

… more powerful `[python].resolves_to_only_binary` and `[python].resolves_to_no_binary` (pantsbuild#16513) Part of the per-resolve config project at https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit. We already with multiple resolves allow you to have conflicting versions of the same requirement, e.g. Django 2 vs Django 3. So, it's useful to also allow those resolves to set different options for `--no-binary` and `--only-binary`, as you might only need it for certain versions of a project or for certain contexts. This only works with Pex lockfiles, with similar reasoning to why we closed pantsbuild#16476. This adds the options to the lockfile header, making progress on pantsbuild#12832.

[python].resolves_to_constraints_file works without lockfiles and n…

3a9ebc5

…on-Pex lockfiles # 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]

Eric-Arellano added the category:internal CI, fixes for not-yet-released features, etc. label Aug 11, 2022

Eric-Arellano requested review from jsirois and benjyw August 11, 2022 03:33

Eric-Arellano commented Aug 11, 2022

View reviewed changes

Eric-Arellano force-pushed the constraints-file-always-used branch from 7264ade to 3a9ebc5 Compare August 11, 2022 03:37

jsirois reviewed Aug 11, 2022

View reviewed changes

Eric-Arellano added 2 commits August 11, 2022 09:29

Do not hook up pep-517-build-backend to constraints files

15af41d

# 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]

Fix tests

29fa094

# 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]

Eric-Arellano added 5 commits August 11, 2022 11:00

Merge branch 'main' of github.com:pantsbuild/pants into constraints-f…

9e34ef3

…ile-always-used [ci skip-build-wheels]

Remove TODO about local_dists.py

073bd19

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Revert global-plugins-option, no-user-resolve, and `pep-517-build…

30434d9

…-backend` # 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]

Don't mislead in comments

25504e2

# 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]

Remove bad f string

0a14714

# 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]

Eric-Arellano requested a review from jsirois August 11, 2022 18:22

Eric-Arellano changed the title ~~[python].resolves_to_constraints_file works without lockfiles and non-Pex lockfiles~~ [python].resolves_to_constraints_file works with non-Pex lockfiles and without lockfiles Aug 11, 2022

Eric-Arellano commented Aug 11, 2022

View reviewed changes

benjyw approved these changes Aug 12, 2022

View reviewed changes

Eric-Arellano closed this Aug 12, 2022

Eric-Arellano deleted the constraints-file-always-used branch August 12, 2022 20:34

Eric-Arellano mentioned this pull request Aug 14, 2022

Deprecate [python].only_binary and [python].no_binary in favor of more powerful [python].resolves_to_only_binary and [python].resolves_to_no_binary #16513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`[python].resolves_to_constraints_file` works with non-Pex lockfiles and without lockfiles #16476

`[python].resolves_to_constraints_file` works with non-Pex lockfiles and without lockfiles #16476

Eric-Arellano commented Aug 11, 2022 •

edited

Loading

Eric-Arellano Aug 11, 2022

Eric-Arellano Aug 11, 2022

jsirois Aug 11, 2022 •

edited

Loading

Eric-Arellano Aug 11, 2022

jsirois Aug 11, 2022

jsirois commented Aug 11, 2022 •

edited

Loading

jsirois left a comment

jsirois Aug 11, 2022

Eric-Arellano Aug 11, 2022

jsirois Aug 11, 2022

Eric-Arellano Aug 11, 2022

jsirois Aug 11, 2022

Eric-Arellano Aug 11, 2022

jsirois Aug 11, 2022

Eric-Arellano Aug 11, 2022 •

edited

Loading

Eric-Arellano commented Aug 11, 2022

Eric-Arellano Aug 11, 2022

benjyw Aug 12, 2022

Eric-Arellano commented Aug 12, 2022 •

edited

Loading

[python].resolves_to_constraints_file works with non-Pex lockfiles and without lockfiles #16476

[python].resolves_to_constraints_file works with non-Pex lockfiles and without lockfiles #16476

Conversation

Eric-Arellano commented Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois commented Aug 11, 2022 • edited Loading

jsirois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Eric-Arellano commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Aug 12, 2022 • edited Loading

`[python].resolves_to_constraints_file` works with non-Pex lockfiles and without lockfiles #16476

`[python].resolves_to_constraints_file` works with non-Pex lockfiles and without lockfiles #16476

Eric-Arellano commented Aug 11, 2022 •

edited

Loading

jsirois Aug 11, 2022 •

edited

Loading

jsirois commented Aug 11, 2022 •

edited

Loading

Eric-Arellano Aug 11, 2022 •

edited

Loading

Eric-Arellano commented Aug 12, 2022 •

edited

Loading