https://pantsbuild.org/ logo
Powered by
h

hundreds-father-404

03/17/2020, 3:58 PM
Hi, would you expect graph ambiguity for this?
Copy code
X -> Z
A -> X -> Z
B -> X -> Z
Specifically:
Copy code
StripSnapshotRequest -> SourceRootStrippedSources
StripSourcesFieldRequest -> StripSnapshotRequest -> SourceRootStrippedSources
LegacyStripTargetRequest -> StripSnapshotRequest -> SourceRootStrippedSources
a

aloof-angle-91616

03/17/2020, 3:59 PM
does 
A -> X -> Z
not just call the same function used in 
X -> Z
?
h

hundreds-father-404

03/17/2020, 4:00 PM
A -> X -> Z
ends with the line 
return await Get[Z](X, x)
Specifically, 
return await Get[SourceRootStrippedSources](StripSnapshotRequest(..))
Same thing for 
B -> X -> Z
. (I need both 
A
and 
B
to be able to incrementally roll out the target API)
a

aloof-angle-91616

03/17/2020, 4:02 PM
i can't see yet where the ambiguity would arise, personally, might be missing something
a

average-vr-56795

03/17/2020, 4:05 PM
Are you seeing ambiguity? What’s the error message? 🙂
👍 1
h

hundreds-father-404

03/17/2020, 4:06 PM
a

aloof-angle-91616

03/17/2020, 4:08 PM
does the same thing happen if you do
Copy code
x = await Get[SourceRootStrippedSources](...)
return x
?
the ast visitor we use to find 
Get
types might need to be fixed to recognized 
return await Get(...)
statements, not sure
h

hundreds-father-404

03/17/2020, 4:09 PM
does the same thing happen if you do
It does, same error. Good intuition though - thanks!
a

aloof-angle-91616

03/17/2020, 4:10 PM
do you have a previously working version to bisect from?
h

hundreds-father-404

03/17/2020, 4:10 PM
In the Python file I sent, comment out line 196 that registers 
strip_source_roots_from_sources_field
and there’s no more ambiguity
a

aloof-angle-91616

03/17/2020, 4:10 PM
ok
h

hundreds-father-404

03/17/2020, 4:11 PM
So, we do allow this:
Copy code
X -> Z
B -> X -> Z
The issue is adding 
A -> X -> Z
a

aloof-angle-91616

03/17/2020, 4:11 PM
ah
thank you, i see why the error happens now
💯 1
👖 1
there are two ways i can see to try to address this: (1) when creating the 
RuleIndex
, look for rules that create this 
B -> X -> Z
ambiguity, and introduce intermediate anonymous `@rule`s to disambiguate
so we could reach into the 
TaskRule
created by e.g. 
strip_source_roots_from_snapshot
, and make it return an anonymous 
@dataclass
which wraps 
SourceRootStrippedSources
, and then add a rule to convert that anonymous 
@dataclass
into 
SourceRootStrippedSources
(trying to find an easier way to describe this)
i think this is possible, and may work. thinking about whether there's an easier way
a

average-vr-56795

03/17/2020, 4:17 PM
@aloof-angle-91616 This is a bug though, right? It’s not intended to be ambiguous?
a

aloof-angle-91616

03/17/2020, 4:17 PM
yess
a

average-vr-56795

03/17/2020, 4:17 PM
Cool, just wanted to check 🙂
a

aloof-angle-91616

03/17/2020, 4:18 PM
i was trying to figure out ways for the engine to fix up the rule graph so that it accepts these rules as written
and was thinking we could do some doctoring in the place where we create a 
RuleIndex
h

hundreds-father-404

03/17/2020, 4:19 PM
Interesting, I don’t follow why both 
A -> X -> Z
and 
B -> X -> Z
creates ambiguity, though? For now, I think I can resolve this by creating a new type 
LegacyStrippedSourceFiles
, i.e.
Copy code
X -> Z
A -> X -> Z
B -> X -> Z -> M
That might not actually work, but I think I can find a way to hack it through new types
a

average-vr-56795

03/17/2020, 4:19 PM
Do you understand what the root cause of the problem is here? It feels like introducing intermediate rules wouldn’t fundamentally change anything
h

hundreds-father-404

03/17/2020, 4:19 PM
Should I open a ticket about this? I don’t understand what causes the bug, but could document the high level description and then you both add additional context?
a

aloof-angle-91616

03/17/2020, 4:20 PM
@hundreds-father-404 the "anonymous dataclass" i was thinking of would be more like:
Copy code
X -> Z
A -> M
M -> X
B -> N
N -> X
where M and N are wrapper types
Should I open a ticket about this?
yes!
💯 1
i think @average-vr-56795 correctly identified what should be working
and i would put this as an "open question":
Do you understand what the root cause of the problem is here? It feels like introducing intermediate rules wouldn’t fundamentally change anything
h

hundreds-father-404

03/17/2020, 4:24 PM
Please feel free to change the title or description! https://github.com/pantsbuild/pants/issues/9320 Thanks for talking this over!
a

aloof-angle-91616

03/17/2020, 4:25 PM
this is weird behavior! thanks for being so thorough!
❤️ 1
w

witty-crayon-22786

03/17/2020, 4:46 PM
sorry for the trouble. i have a fairly good idea of how to improve these errors that i need to prioritize
currently we're "failing slow" with regard to errors, but also rendering not just what is probably the root cause, but everything above it
this felt like it made sense when the graph was smaller. now that it is larger, it's clearly less helpful.
h

hundreds-father-404

03/17/2020, 4:55 PM
currently we're "failing slow" with regard to errors, but also rendering not just what is probably the root cause, but everything above it
That’d be wonderful! We’re hoping to very soon get some users writing their own V2 plugins (roughly once the dust settles on the target API), so we’re very eager to improve the rule authoring experience.
w

witty-crayon-22786

03/17/2020, 5:12 PM
@hundreds-father-404: the snippet of the error above isn't really usable because the indentation was lost
(in the section related to ambiguity, in particular)
or is
Copy code
Ambiguous rules to compute Get[SourceRootStrippedSources](StripSnapshotRequest) with parameter type LegacyStripTargetRequest:
      @rule(LegacyStripTargetRequest) -> SourceRootStrippedSources,
gets=[Get[SourceRootStrippedSources](StripSnapshotRequest)]
pants.rules.core.strip_source_roots:166:legacy_strip_source_roots_from_target
for ()
      @rule(StripSourcesFieldRequest) -> SourceRootStrippedSources,
gets=[
Get[SourcesResult](SourcesRequest)
Get[SourceRootStrippedSources](StripSnapshotRequest),
]
...what we are rendering now post http://github.com/pantsbuild/pants/pull/7024 ...? oof
Powered by