Support for using a single global resolve. #10405

benjyw · 2020-07-19T20:45:19Z

Introduces a --python-setup-use-all-requirements flag.
If set, instead of resolving just the exact requirements
needed, we resolve the entire lockfile.

This prevents a huge amount of resolving work of all
the various requirement subsets needed by specific tests.

[ci skip-rust-tests]

Introduces a --python-setup-use-all-requirements flag. If set, instead of resolving just the exact requirements needed, we resolve the entire lockfile. This prevents a huge amount of resolving work of all the various requirement subsets needed by specific tests. [ci skip-rust-tests]

coveralls · 2020-07-19T21:35:29Z

Coverage remained the same at 0.0% when pulling 881ec72 on benjyw:resolve_everything into 669b5f3 on pantsbuild:master.

benjyw · 2020-07-20T17:05:46Z

PS feel free to bikeshed the flag name

src/python/pants/python/python_setup.py

Eric-Arellano

Thank you!

Eric-Arellano · 2020-07-20T17:47:32Z

src/python/pants/python/python_setup.py

+            # TODO: Should this default to True? Seems like a better initial experience for most
+            #  users (although they still require at least a trivial lockfile to get any benefit).
+            default=False,


I think no, in part for that reason of needing a lockfile configured. Instead, we can suggest it in the docs.

src/python/pants/python/python_setup.py

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

Eric-Arellano · 2020-07-20T17:52:12Z

src/python/pants/backend/python/rules/pex.py

+        # Note that if a request has both requirements and requirements_files, the description
+        # will only mention the requirements. However we have no such case today anyway.


See below on possibly merging this.

src/python/pants/python/python_setup.py

# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]

[ci skip-rust-tests]

benjyw · 2020-07-20T21:29:40Z

OK so now we don't apply the behavior unless we confirm that the reqs are a subset of those in the lockfile .(at the project name level)

benjyw · 2020-07-20T21:31:23Z

@stuhood does this address the concern? Namely this change still doesn't have an opinion about how to pick a lockfile, but if one is picked, it will apply this behavior in that context - only if the reqs are a subset (so for example if no single lockfile is possible, something sensible will still happen).

stuhood

I think that we should try to eliminate this setting in the future by removing the downside (namely, that all of the requirements from the lockfile file are present). If we have time to implement that approach now, that would be preferable.

The existence of a lockfile implies that there is a globally consistent resolve associated with that lockfile (ie, that it's possible to choose exactly one version of each library). If that is the case, then targets that depend on that resolve should be able to consume only the relevant subset of the resolve... consuming the "relevant subset" could be accomplished by having either pip or pex emit a graph of the dependencies within the resolve, allowing individual targets to depend on a subset of the artifacts, while still depending on the same single resolve associated with their lockfile. That would avoid invalidating tests/binaries for irrelevant artifacts.

stuhood · 2020-07-21T00:22:06Z

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

+        if exact_req_projects.issubset(constraint_file_projects):
+            requirements = PexRequirements(str(req) for req in constraints_file_reqs)
+        else:
+            requirements = exact_reqs


This would mean that the lockfile was incorrectly or partially created, I think? That probably justifies a warning.

That could possibly the case. Or they may intentionally be using an inline requirement and they don't want it included in the global constraints file. I'd bias towards no warning (noisy), but will defer to you both.

Given that 1) creating a lockfile is currently a manual process that someone can get wrong, and 2) the reason that you would do it would be to give you build determinism... I think that it makes sense to warn here, at the very least.

But perhaps it is the case that that warning should trigger regardless of the --resolve-all-constraints setting, anytime that you have a lockfile configured and we encounter a requirement that is not specified in the constraints file? This is in the general category of "things are pretty manual right now" as described in https://www.pantsbuild.org/docs/python-third-party-dependencies#using-a-lockfile-recommended

Good points. That makes sense and fits with a theme I'd like for Pants to have of Pants eagerly helping you to do what you likely intended, kind of like how people describe the Rust compiler.

Yeah, I think this makes sense as a warning. Will add.

pants.toml

src/python/pants/python/python_setup.py

Eric-Arellano

This looks great! Thank you for making the change with inline requirements!

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

benjyw · 2020-07-21T00:55:05Z

I think that we should try to eliminate this setting in the future by removing the downside (namely, that all of the requirements from the lockfile file are present). If we have time to implement that approach now, that would be preferable.

The existence of a lockfile implies that there is a globally consistent resolve associated with that lockfile (ie, that it's possible to choose exactly one version of each library). If that is the case, then targets that depend on that resolve should be able to consume only the relevant subset of the resolve... consuming the "relevant subset" could be accomplished by having either pip or pex emit a graph of the dependencies within the resolve, allowing individual targets to depend on a subset of the artifacts, while still depending on the same single resolve associated with their lockfile. That would avoid invalidating tests/binaries for irrelevant artifacts.

This might be more complicated, and I would rather punt on it for now. For example, creating the relevant subset means, I assume, symlink farming. Are we sure we can always get that right?

stuhood · 2020-07-21T00:59:28Z

For example, creating the relevant subset means, I assume, symlink farming.

It would mean having pex or pip emit this information. https://pypi.org/project/pipdeptree/ exists, although it might do symlink farming behind the scenes. But also, I recall that @jsirois had talked about extracting this info from wheel metadata in order to render better error messages, so that's another alternative.

benjyw · 2020-07-21T02:42:37Z

For example, creating the relevant subset means, I assume, symlink farming.

It would mean having pex or pip emit this information. https://pypi.org/project/pipdeptree/ exists, although it might do symlink farming behind the scenes. But also, I recall that @jsirois had talked about extracting this info from wheel metadata in order to render better error messages, so that's another alternative.

The info is only half the problem. How do you actually create a chroot with just the relevant dists and no others?

[ci skip-rust-tests]

stuhood · 2020-07-21T04:05:41Z

The info is only half the problem. How do you actually create a chroot with just the relevant dists and no others?

Unclear. A filtering operation on a pex? Or perhaps after splitting resolve from install, there will be other intermediate artifacts (...perhaps just wheels).

[ci skip-rust-tests]

benjyw · 2020-07-21T04:47:25Z

OK, added the warning when the constraints don't cover the requirements (in all cases, regardless of the new flag), and added a test.

benjyw requested review from Eric-Arellano, stuhood and jsirois July 19, 2020 20:45

benjyw mentioned this pull request Jul 19, 2020

Resolving requirements performance is still not acceptable #10380

Closed

benjyw force-pushed the resolve_everything branch from e639d0c to 81d58f6 Compare July 19, 2020 20:53

stuhood reviewed Jul 20, 2020

View reviewed changes

src/python/pants/python/python_setup.py Outdated Show resolved Hide resolved

Eric-Arellano approved these changes Jul 20, 2020

View reviewed changes

benjyw added 2 commits July 20, 2020 14:10

Address code review comments

643647f

# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]

Only use the functionality if all reqs are in the lockfile.

e3cec26

[ci skip-rust-tests]

stuhood approved these changes Jul 21, 2020

View reviewed changes

stuhood reviewed Jul 21, 2020

View reviewed changes

src/python/pants/python/python_setup.py Outdated Show resolved Hide resolved

src/python/pants/python/python_setup.py Outdated Show resolved Hide resolved

Eric-Arellano approved these changes Jul 21, 2020

View reviewed changes

src/python/pants/backend/python/rules/pex_from_targets.py Show resolved Hide resolved

Always warn if constraints don't cover requirements.

b993c4b

[ci skip-rust-tests]

Test

881ec72

[ci skip-rust-tests]

benjyw merged commit 45ca5b4 into pantsbuild:master Jul 21, 2020

stuhood mentioned this pull request Jul 27, 2020

Constraints file incompleteness warning added in #10405 is hard to avoid #10473

Closed

benjyw deleted the resolve_everything branch August 7, 2020 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for using a single global resolve. #10405

Support for using a single global resolve. #10405

benjyw commented Jul 19, 2020

coveralls commented Jul 19, 2020 •

edited

benjyw commented Jul 20, 2020

Eric-Arellano left a comment

Eric-Arellano Jul 20, 2020

Eric-Arellano Jul 20, 2020

benjyw commented Jul 20, 2020

benjyw commented Jul 20, 2020

stuhood left a comment •

edited

stuhood Jul 21, 2020

Eric-Arellano Jul 21, 2020

stuhood Jul 21, 2020

Eric-Arellano Jul 21, 2020

benjyw Jul 21, 2020

Eric-Arellano left a comment

benjyw commented Jul 21, 2020

stuhood commented Jul 21, 2020

benjyw commented Jul 21, 2020

stuhood commented Jul 21, 2020

benjyw commented Jul 21, 2020

		# Note that if a request has both requirements and requirements_files, the description
		# will only mention the requirements. However we have no such case today anyway.

Support for using a single global resolve. #10405

Support for using a single global resolve. #10405

Conversation

benjyw commented Jul 19, 2020

coveralls commented Jul 19, 2020 • edited

benjyw commented Jul 20, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Jul 20, 2020

Choose a reason for hiding this comment

Eric-Arellano Jul 20, 2020

Choose a reason for hiding this comment

benjyw commented Jul 20, 2020

benjyw commented Jul 20, 2020

stuhood left a comment • edited

Choose a reason for hiding this comment

stuhood Jul 21, 2020

Choose a reason for hiding this comment

Eric-Arellano Jul 21, 2020

Choose a reason for hiding this comment

stuhood Jul 21, 2020

Choose a reason for hiding this comment

Eric-Arellano Jul 21, 2020

Choose a reason for hiding this comment

benjyw Jul 21, 2020

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

benjyw commented Jul 21, 2020

stuhood commented Jul 21, 2020

benjyw commented Jul 21, 2020

stuhood commented Jul 21, 2020

benjyw commented Jul 21, 2020

coveralls commented Jul 19, 2020 •

edited

stuhood left a comment •

edited