https://pantsbuild.org/ logo
Join Slack
Powered by
I fleshed out the design for how to integrate gene...
# development
h
I fleshed out the design for how to integrate generated subtargets with file arguments, dependency ignore syntax, and explicit file-level deps. Please take a look: https://docs.google.com/document/d/14MAst0Q0HpoLqysiN7KoKDkA6-Ga_KhjICTl0EC16uI/edit#heading=h.y495n7exlx1x
cc @witty-crayon-22786 @happy-kitchen-89482
w
thanks, will try to take a look this afternoon
h
It’s fairly short if you have a moment before then. In the meantime, I’ll look into the 1.30 PR and can start docs on upgrading to 2.0
w
@hundreds-father-404: which part? the 
Generated Subtargets fleshed out
section?
h
Yes, particularly the “Proposal” part
you can skip “Key questions”. That was more me thinking out loud
Stu, any thoughts on point 2c about relative file names? I’m not sure what the right answer is.
w
…seems like yes. and it seems like that might influence the syntax.
h
I think agree the answer is yes, mostly for alignment with the 
sources
field
w
alignment with sources would be nice to have, but is trickier maybe.
and we’re pretty sure we’re drawing the line well-before “file globs in dependencies lists”
👍 1
h
Well, yes, not full alignment. 
Dependencies
should only be literal file names and/or literal addresses. No globs.
w
right. so i think that the more important alignment is with target addresses, which have a relative syntax
👍 1
h
True. So, really using relative file names means alignment with both sources and target addresses
w
had one more thought about the proposal, which is just that in each of the three contexts where we construct a file address, the behavior is different, and that’s Ok as long as there is alignment in each of those cases… 1) literal address in a BUILD file, 2) inferred dependency, 3) CLI spec
reading it now, it looks like these three cases are already broken out, and the behavior is specified
👍 1
and i think that what you’ve proposed for each makes sense. 1) literals in BUILD files are literals: if they are not specific enough, they should fail (same as an address should), 2) inference is choosing which of some concrete/literal set of choices to use: refusing to do that makes sense, 3) Specs have always resulted in 0 or more addresses, so matching “all” candidates makes sense
💯 1
h
Agreed. The multiple owners behavior is the main thing that differs between the three: - dep inference -> no-op - literal address in BUILD file -> error (for now at least) - CLI -> run on all the owners, like we do now
coke
w
👍
but, that does mean that we will need syntax to make a literal in a BUILD file unambiguous. how soon is unclear: possibly immediately.
(because you need it both for both ambiguity, and for false positives)
h
I’m not convinced that’s a priority. You can use a traditional explicit target in that case that we error because of ambiguity. The downside is that you may possibly have coarser caching than if you were using a generated subtarget. But that’s not the end of the world; it’s only not as fine-grained of caching as we’d prefer. Cycles are not relevant, given your upcoming cycle tolerance PR. -- I don’t think false positives come into play here. Because we no-op with dep inference on ambiguity, we would not have a false positive. If anything, it’s a false negative.
w
a false positive is “i didn’t want the dep, but i got it”
that can happen without ambiguity
☝️ 1
h
Agreed. How would that happen if dep inference no-ops?
that can happen without ambiguity
Yes, but then you can ignore it like you normally would via 
ignore_me.py
. There’s zero ambiguity what that means because there is only one owner. We don’t need a syntax to disambiguate between multiple owners.
w
hm. possibly.
h
So long as dep inference continues to no-op on multiple owners, I don’t see how we could ever have a false positive that is ambiguous.
w
er, to be clear: i’m saying it can happen without ambiguity
and you are correctly saying that in the case you could use the file, with no need to disambiguate
👍 1
h
A false positive can happen without ambiguity? Agreed. You use the file name for that.
w
so yea, i think this whole “is there ambiguity” thing is the razor by which we decide whether to {require, render} the owning target as part of the syntax.
and i think that the “render” bit applies to your 5c
h
Further, in writing the PR description, we might actually not want to always use generated subtargets for file arguments (point 5 of the proposal). We could I think refactor to detect this, and use the original owning targets with distinct address representations This would solve the problem of 
helloworld/app.py
showing up multiple times in the output, even though they are actually different generated subtargets.
w
not sure if you were responding to what i was saying, but if not, coke again?
h
coke again hehe
w
i think the difference between what we each said is that i’m suggesting that the Address object itself might be different if we detect ambiguity
h
I feel really strongly that we do not want to have 
helloworld/app.py@original_owner
show up in the output of Pants. And as pointed out in the gdoc, we couldn’t special case when to show that verbose representation. It would need to always be verbose So, I think the best solution is to have file args not use generated subtargets if there’s ambiguity. In general, we can discourage users from using multiple owners
w
and if the object is different, it’s rendered differently.
I feel really strongly that we do not want to have 
helloworld/app.py@original_owner
 show up in the output of Pants.
if that is what someone will (eventually) need to put in a BUILD file to select that file unambiguously, then it seems consistent to.
i agree that we don’t always want to do it… that would be noisy. but see my last.
h
Well, if we want to support that feature. Unclear to me imo. We may say in that case that you need to use explicit targets like before
I’m not sure if introducing a new concept (
@
) would be worth it for this one edge case. There are already lots of new concepts we’re introducing.
In general, I advocate keeping it simple for now, and we can choose to make it more flexible if we find users need it. This design should allow for a future addition of the feature: - explicit BUILD files -> error - dep inference -> no-op - CLI -> don’t generate subtargets and use all the original owners
In the future, this would be: - explicit BUILD files -> error unless using the disambiguation - dep inference -> no-op - CLI -> generate subtargets for each of the original owners
w
sounds right. but i do think that syntax is the right way to resolve 5c here. if it’s easier to defer and do later then that’s fine
ok, thanks a lot!… off to other things.
h
Agreed with that iff we decide we want to support this feature. Thanks for talking it over. That resolved the last remaining issue here afaict 🙂
w
@hundreds-father-404: er, sorry. it’s relevant regardless of whether we support the feature because of 5c
right?
anyway, minor
h
No, because 5c will never happen if we do all our call sites right. Per https://pantsbuild.slack.com/archives/C0D7TNJHL/p1594672010035500?thread_ts=1594662462.023500&cid=C0D7TNJHL, we would never have a case where there are multiple generated subtargets referring to the same file
w
5c is referring to file arguments
h
Indeed.
- CLI -> don’t generate subtargets and use all the original owners
w
ok, i think i was misunderstanding then. that’s a change from what we had previously discussed
IMO, what the doc said before was better, assuming we have a way to render the ambiguous case.
h
Yes, to allow us to punt on deciding a new syntax until we have more user feedback and data on how people are using these new concepts
w
ok. if that’s explicit, then it’s fine with me.
h
assuming we have a way to render the ambiguous case
This would imply, imo, that what we render also works in BUILD files. Which is what I want to defer.
When there are multiple owners, file args will behave like they do today
w
thanks
❤️ 1
h
It’s not perfect - it means we need to keep abstractions around like 
SpecifiedSourcsSnapshot
, which I really was excited to remove. But I think it’s worth it because adding new syntax is a much bigger deal than cleaning up some internal abstractions
w
yea.
Open in Slack
PreviousNext
Powered by