Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix some dependencies-like fields not showing up with project introspection #10923
Fix some dependencies-like fields not showing up with project introspection #10923
Changes from all commits
143288b
f8fb91d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't think this would be incredibly difficult to move to a separate rule for
DependenciesWithIndirectDeps
, which would append thespecial_cased
to the result as is done 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.
If we added a new rule, we would probably want its result type to be
Addresses
to avoid having to invent more wrapper dataclasse. But, that will result in graph ambiguity to useawait Get(Addresses, Foo)
in this rule, unless we added the new dataclass.I don't think there's strong enough of motivation yet to factor this out and pay the cost of Yet Another Dataclass and rule. Note that we don't really expect call sites to resolve all their special deps in one single call, without their normal deps included. Instead, we expect them to resolve each distinct field separately, because each field has its own magical handling.
archive
is a good example of this:pants/src/python/pants/core/target_types.py
Lines 295 to 304 in ea542ac
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'm not sure I understand. If this is the case, why aren't we separating the calls for
Dependencies
andSpecialCasedDependencies
then?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.
Because there are very select places that we want all dependencies, both normal and special cased. Specifically, project introspection needs all dependencies.
It is simpler in those places to have one call
await Get(Addresses, DependenciesRequest)
than a call for normal deps + call for all the special cased. In particular, this is apparent with TransitiveTargets. With this current design, we don't need to implement a separate transitive targets rule that duplicates the normal rule. It sounds like you tried implementing new distinct dataclasses yesterday and found this to be the case?My above comment was trying to explain that in almost all other contexts, you do not want all dependencies, both normal and magical, to be in the same result. You want to separate them out. I was pointing to how that it possible with that design.
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 have been extremely trivial if #8542 had been merged, which would have allowed providing a
@union
base as an@rule
argument. We could have then made aTransitiveTargetsRequestBase
union, and it would have generated a version of that rule for each type. I have asked you and others to take a look at that PR many times to no avail. Would you consider looking at it again if I updated it?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'll take a look at #8542 and comment there
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.
Could we link to #10888 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.
Actually, could we add a TODO pointing to #10924?
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 don't think there's any TODO we need to do here, tbh. This is idiomatic Python and idiomatic usage of the Target API. We get the functionality we need in 4 lines of code, without needing to invent any new abstractions.
The comment is solely to draw attention to why we don't use
tgt.get()
like normal.Is there something you think we could improve with these 4 lines?
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 it's just confusing to me why we would iterate over the target's fields again here instead of making that part of the original
DependenciesRequest
. I think it was a good decision to make sure eachField
has anAddress
, but it feels like that's useful for when we discover new things we need to introspect about the original target. I'm not sure it's really a problem, but it feels like that might be part of why the graph cycle is happening 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.
I'm not really following the argument here. At this point, we had only discovered that there is a
Dependencies
field (or subclass). We need to check if there are any other special cased dependencies fields that are in play, and the way to do that is to iterate over all the fields to see if they subclass this marker.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.
What if we included the special-cased dependencies fields in the original
DependenciesRequest
?