-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
Add [python].resolves_to_constraints_file
#16420
Add [python].resolves_to_constraints_file
#16420
Conversation
# 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]
Exactly! Let Pip do so and do not nanny tools if you don't need to. But that's neither here nor there, I'm just continually pointing out poor argument noise when I hear it. I think its fine to start with just constraints files as a whole. Synthesizing them from other configuration sources later and letting the proper nanny (Pip) do the nannying, is always an option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, including constraints but not invalidating the lock based on constraints changing is a bug.
[ci skip-rust] [ci skip-build-wheels]
"""Configuration from `[python]` that impacts how the resolve is created.""" | ||
|
||
constraints_file: tuple[Digest, FileEntry] | None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will soon also have --only-binary
and --no-binary
. Possibly [python-repos]
options, pending Slack discussion
if isinstance(self, cls): | ||
# Note that we do exact version matches so that authors can subclass earlier versions. | ||
if type(self) is cls: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chrisjrn for the fix
all_tool_resolve_names = tuple( | ||
sentinel.resolve_name for sentinel in union_membership.get(GenerateToolLockfileSentinel) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird. It means that our error message for unrecognized resolve names is going to include JVM tools like javac. I could not think of a good way to filter down to only Python tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its also a bit weird in the small that you could ask for ResolvePexConfig for the resolve "junit" and get back a non-error answer at all. It definitely seems like a level of typing structure is missing from the infra here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pip_args_file = "__pip_args.txt" | ||
extra_args.extend(["-r", pip_args_file]) | ||
pip_args_file_content = "\n".join( | ||
[f"--no-binary {pkg}" for pkg in python_setup.no_binary] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was pre-existing, but changing the order of the python_setup
args produces a new file here and the subsequent downstream invalidations that flow when it need not, --no-binary
/ --only-binary
is not an order-sensitive thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was pre-existing
My next PR will likely be reworking this mechanism to be resolve-specific, so I can clean that up there.
all_tool_resolve_names = tuple( | ||
sentinel.resolve_name for sentinel in union_membership.get(GenerateToolLockfileSentinel) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its also a bit weird in the small that you could ask for ResolvePexConfig for the resolve "junit" and get back a non-error answer at all. It definitely seems like a level of typing structure is missing from the infra here.
@@ -306,6 +381,9 @@ def validate_metadata( | |||
lockfile=lockfile, | |||
user_interpreter_constraints=interpreter_constraints, | |||
user_requirements=user_requirements, | |||
maybe_constraints_file_path=constraints_file_path_and_hash[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types certainly mean 0 must be the correct index, but its definitely not reader-friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I considered using NamedTuple
, but felt confusing to introduce yet another class when we already have so many. So I was hoping the path_and_hash[0]
would be clear enough it means path
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I expressed my thoughts! I'd personally break out a 2 field dataclass, but I ship-it-ed because this is all fashion and not too important to the end goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second that this should be a dataclass.
constraints_file: tuple[Digest, FileEntry] | None | ||
|
||
@property | ||
def constraints_file_path_and_hash(self) -> tuple[str, str] | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if this returned a dataclass and not a tuple whose unpacking is not self-explanatory. Reading the code that uses this was confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this can wait for a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
John agreed with you. Will do :)
@@ -306,6 +381,9 @@ def validate_metadata( | |||
lockfile=lockfile, | |||
user_interpreter_constraints=interpreter_constraints, | |||
user_requirements=user_requirements, | |||
maybe_constraints_file_path=constraints_file_path_and_hash[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second that this should be a dataclass.
…bclasses (#16449) Closes #16443, which is necessary for the per-resolve config project from https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit# to work properly. See #16420 (comment). As discussed on the ticket, this is not a very elegant solution. But we don't have the rule graph introspection mechanism necessary to automagically figure out what type each tool is, and that also seems brittle. As explained in this PR's updated docs, plugin authors don't have to technically make this change. [ci skip-rust] [ci skip-build-wheels]
Improves upon #16420. @huonw wisely suggested we avoid using `hash` for constraints files because it makes Git diffs and merge conflicts much worse. python-poetry/poetry#496 rust-lang/cargo#7070 [ci skip-rust] [ci skip-build-wheels]
Constraints files are extremely useful for pinning problematic transitive deps. We abused constraints files early on as a poor-man's lockfile via `[python].requirement_constraints`. While we want to deprecate that in favor of `[python].resolves`, it is useful to allow constraint files for resolves. Specifically, each resolve should be allowed its own constraints file. For example, if resolve A uses Django 2, it may need to pin different problematic transitive deps of Django than resolve B using Django 3. -- This PR is the first out of a project to allow specifying config for lockfiles on a per-resolve basis, rather than globally for all resolves: https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit#. -- This requires us now tracking `constraints_file_hash` in the lockfile metadata, so that when the user changes a constraints file, we know to regenerate the lock. Note that this means we have a V3 of lockfile metadata. It is okay to keep using V2 -- the next time people run `generate-lockfiles`, they will be bumped. [ci skip-rust] [ci skip-build-wheels]
…bclasses (pantsbuild#16449) Closes pantsbuild#16443, which is necessary for the per-resolve config project from https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit# to work properly. See pantsbuild#16420 (comment). As discussed on the ticket, this is not a very elegant solution. But we don't have the rule graph introspection mechanism necessary to automagically figure out what type each tool is, and that also seems brittle. As explained in this PR's updated docs, plugin authors don't have to technically make this change. [ci skip-rust] [ci skip-build-wheels]
…sbuild#16469) Improves upon pantsbuild#16420. @huonw wisely suggested we avoid using `hash` for constraints files because it makes Git diffs and merge conflicts much worse. python-poetry/poetry#496 rust-lang/cargo#7070 [ci skip-rust] [ci skip-build-wheels]
Constraints files are extremely useful for pinning problematic transitive deps.
We abused constraints files early on as a poor-man's lockfile via
[python].requirement_constraints
. While we want to deprecate that in favor of[python].resolves
, it is useful to allow constraint files for resolves.Specifically, each resolve should be allowed its own constraints file. For example, if resolve A uses Django 2, it may need to pin different problematic transitive deps of Django than resolve B using Django 3.
--
This PR is the first out of a project to allow specifying config for lockfiles on a per-resolve basis, rather than globally for all resolves: https://docs.google.com/document/d/1HAvpSNvNAHreFfvTAXavZGka-A3WWvPuH0sMjGUCo48/edit#.
--
This requires us now tracking
constraints_file_hash
in the lockfile metadata, so that when the user changes a constraints file, we know to regenerate the lock.Note that this means we have a V3 of lockfile metadata. It is okay to keep using V2 -- the next time people run
generate-lockfiles
, they will be bumped.[ci skip-rust]
[ci skip-build-wheels]