https://pantsbuild.org/ logo
Join Slack
Powered by
Hi, would you expect graph ambiguity for this? ``...
# development
h
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
does 
A -> X -> Z
not just call the same function used in 
X -> Z
?
h
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
i can't see yet where the ambiguity would arise, personally, might be missing something
a
Are you seeing ambiguity? What’s the error message? 🙂
👍 1
h
a
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
does the same thing happen if you do
It does, same error. Good intuition though - thanks!
a
do you have a previously working version to bisect from?
h
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
ok
h
So, we do allow this:
Copy code
X -> Z
B -> X -> Z
The issue is adding 
A -> X -> Z
a
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
@aloof-angle-91616 This is a bug though, right? It’s not intended to be ambiguous?
a
yess
a
Cool, just wanted to check 🙂
a
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
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
Do you understand what the root cause of the problem is here? It feels like introducing intermediate rules wouldn’t fundamentally change anything
h
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
@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
Please feel free to change the title or description! https://github.com/pantsbuild/pants/issues/9320 Thanks for talking this over!
a
this is weird behavior! thanks for being so thorough!
❤️ 1
w
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
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
@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
Open in Slack
PreviousNext
Powered by