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

Why are there //:pants-requirements.txt dependencies? #1

Closed
jsirois opened this issue Dec 9, 2021 · 9 comments
Closed

Why are there //:pants-requirements.txt dependencies? #1

jsirois opened this issue Dec 9, 2021 · 9 comments

Comments

@jsirois
Copy link

jsirois commented Dec 9, 2021

"//:pants-requirements.txt",

@benjyw
Copy link
Sponsor Contributor

benjyw commented Dec 9, 2021

I think because the user that generated this out of their real repo used ./pants dependencies to generate the BUILD files.

@benjyw
Copy link
Sponsor Contributor

benjyw commented Dec 9, 2021

So these deps are explicit, but were generated from the inferred deps in a real repo.

@jsirois
Copy link
Author

jsirois commented Dec 9, 2021

Ok. So then part of all this is simply the v1 macros problem. Instead of depending on python_requirements / python_requirement_libraries only, there is a dependence on the file that generates them. As such, rules that act on dependencies will get triggered to re-run for any whitespace change.

@jsirois jsirois closed this as completed Dec 9, 2021
@stuhood
Copy link
Sponsor Member

stuhood commented Dec 9, 2021

pantsbuild/pants#12915 is the relevant ticket then.

@jsirois
Copy link
Author

jsirois commented Dec 9, 2021

Actually, pantsbuild/pants#12915 aside, we have a second over-invalidation in this case since the pnats-requirements.txt file is set up as a psuedo constraints lockfile, and that file, in whole, is part of the input digest of the resolve process:

[python]
interpreter_constraints = [">=3.8,<3.9"]
requirement_constraints = "pants-requirements.txt"

As such, we fully invalidate the cached resolve process on any edit to pants-constraints.txt for 2 independent reasons.

@jsirois
Copy link
Author

jsirois commented Dec 9, 2021

So a pure-annoyance optimization, since we already fully parse the constraints file anyhow, is to used the parsed constraints to emit a normalized constraints file for the input to the process. This would eliminate whitespace / comment edits in the constraint file invalidating the resolve.

@stuhood
Copy link
Sponsor Member

stuhood commented Dec 9, 2021

So a pure-annoyance optimization, since we already fully parse the constraints file anyhow, is to used the parsed constraints to emit a normalized constraints file for the input to the process. This would eliminate whitespace / comment edits in the constraint file invalidating the resolve.

Yea, that makes sense. Because the macro issue alone would not invalidate the resolve (although it might invalidate tests, etc).

@jsirois
Copy link
Author

jsirois commented Dec 9, 2021

Ok. Small bug in that this is only for the (nearly legacy) psuedo-constraints-lockfile hack. Potentially not worth fixing, but also not hard to fix.

@stuhood
Copy link
Sponsor Member

stuhood commented Jan 3, 2022

So a pure-annoyance optimization, since we already fully parse the constraints file anyhow, is to used the parsed constraints to emit a normalized constraints file for the input to the process. This would eliminate whitespace / comment edits in the constraint file invalidating the resolve.

I see an opportunity/necessity to do this as part of pantsbuild/pants#14053

stuhood added a commit to pantsbuild/pants that referenced this issue Jan 4, 2022
…raints. (#14064)

Some callers (#14058) of `PexRequirements` will soon need to be able to specify explicit constraints for building a PEX which are _not_ the global constraints. Additionally, consuming the `constraints` directly as a file (rather than parsing and re-materializing the file) forces whitespace/comment edits to the constraints file to invalidate all resolves ([reported here](pantsbuild/requirements-perf#1 (comment))).

To fix this, replace `apply_constraints: bool` (for the global constraints) with an explicit list of constraints.

[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
None yet
Projects
None yet
Development

No branches or pull requests

3 participants