https://pantsbuild.org/ logo
Join Slack
Powered by
<@UB2J9BQA0> Do you have a handle on <https://gith...
# development
a
@hundreds-father-404 Do you have a handle on https://github.com/pantsbuild/pants/issues/8733 or looking for a hand?
w
To close the loop on this, I think that using ``UnionMembership`` as Danny was suggesting should work: it would look roughly like:
Copy code
sources = ..
for member in union_membership.members(Type):
  sources = await Get[FormattedSources](Type, member.cls(sources))
...ie, execute a series of formatting @rules in the order they were declared in the union. The order would be stable, but not customizable without more complexity... probably fine.
h
And will this need to be used for goals like test too, once we introduce junit?
w
No: see other response in the channel
šŸ‘ 1
a
thanks for summarizing!!!
The order would be stable, but not customizable without more complexity... probably fine.
agreed! and if it does need to be customizable, i feel like that won't be super difficult to extend 
@union
(?) to do
h
Trying to implement this this week. Check on my understanding: 
TargetWithSources
is a union type and itā€™s fulfilled(?) by nested unions, e.g. 
TargetWithSources
may be fulfilled by 
FormattablePythonTarget
and 
FormattableJavaTarget
Those are unions themselves. Then, tools define their own concrete dataclass like 
IsortFormattableTarget
. 
FormattablePythonTarget
would be fulfilled through union rules with 
IsortFormattableTarget
, 
BlackFormattableTarget
, etc
w
second paragraph looks right.
h
Iā€™m possibly confused by this comment? https://github.com/pantsbuild/pants/issues/8343#issuecomment-559489622 I had a hard time following the conversation between what is proposed, should be done, and is only used by 
--query
a
having unions within unions is not supported, but could be
the relevant part from 
--query
is that you can use the 
UnionMembership
singleton to query all members of a specific 
@union
h
having unions within unions is not supported, but could be
Ah okay, thatā€™s why this prototype fails. Would my summary even work for this problem? Thereā€™s still a path from 
TargetWithSources->FormattablePythonTarget->IsortFormattableTarget
and also a path from 
TargetWithSources->FormattablePythonTarget->BlackFormattableTarget
. Thereā€™s no way to disambiguate which of these two to use.
a
in 
--query
, thatā€™s used to kind of ā€œforward declareā€ those union members, so that the union members can be overloaded to also be used to create instances of themselves with a parse method
looking at the code
h
looking at the code
let me post my WIP! https://github.com/pantsbuild/pants/pull/8794
a
basically you need to be requesting a specific path through the rule graph and in 
--query
this is handled by another level of indirection
ok looking
w
@hundreds-father-404: the explanation for what I think should be done to allow for "multiple rules formatting one target" is in this thread in a comment
a
oh yeah
w
Eating lunch, so not sure whether the comment linked above is from before/after Danny and I talked about it and came up with the comment at the top here
h
This?
To close the loop on this, I think that using ``UnionMembership`` as Danny was suggesting should work: it would look roughly like:
sources = ..
for member in union_membership.members(Type):
sources = await Get[FormattedSources](Type, member.cls(sources))
...ie, execute a series of formatting @rules in the order they were declared in the union. The order would be stable, but not customizable without more complexity... probably fine. I didnā€™t follow
w
Yes
Which part?
h
The proposal still involves creating 
IsortFormattableTarget
and 
BlackFormattableTarget
, right? Are those directly members of the 
TargetWithSources
union? Thereā€™s no intermediate type 
PythonFormattableTarget
?
w
Type
in this example would probably be ... a union containing input types for various formatters... ie, ISortTarget and BlackTarget
You'd wrap some actual python target type in one of those wrappers temporarily to kick off formatting
(as in the example... it just doesn't fill in the union or the members)
h
Is this experiment on the right track? Each tool implements its own wrapper type like 
BlackTarget
and 
IsortTarget
and those are direct members of the union 
rules.core.fmt.TargetWithSources
It fails with this message https://github.com/pantsbuild/pants/pull/8794#issuecomment-564769701, but maybe thatā€™s resolvable by changing the implementation of the core 
fmt
and 
lint
rules as you suggested Stu?
w
this is a new union that is not "TargetWithSources"
h
this is a new union that is not ā€œTargetWithSourcesā€
Why is that / does 
TargetWithSources
have any role still or that should be deleted?
w
and it would need to be python specific, because we would launch into the above loop based on this being "a python target'"
i don't know how 
TargetWithSources
is used right now, but it's pretty abstract, and should likely be replaced with something as part of 4535
so, maybe 
PythonFormattableTarget
... unknown: sorry. not familiar with where the unions are right now
h
Wait Iā€™m confused. Are you envisioning that code snippet living in 
rules/core/fmt.py
or somewhere else? I donā€™t think 
fmt.py
can have any knowledge that the language Python even exists
w
this would need to be in the python backend somewhere, i think.
h
Right now, thereā€™s 
TargetWithSources
, which is used by both the core 
fmt
and 
lint
rules. 
python/targets/formatabble_python_target.py
implements 
FormattablePythonTarget
and registers that as a member of 
TargetWithSources
. That file also registers the Python target adaptors as members of 
TargetWithSources
and it registers rules going from each target adaptor to 
FormattablePythonTarget
. Then, the Python linters ask for a 
FormattablePythonTarget
in their rule signatures
w
"all targets with sources" are not necessarily formattable, so that formulation of TargetWithSources is confusing to me.
h
Agreed. It would be more clear to have 
FormattableTarget
and then 
PythonFormattableTarget
w
yea.
h
This was my first experiment. Go from 
FormattableTarget -> PythonFormattableTarget -> BlackTarget|Flake8Target|IsortTarget
, but it failed because we canā€™t have nested unions
w
... need to push on 4535 to make the underlying classes (like 
Target(v2)
) more useful so that not everything needs to be in unions
@hundreds-father-404: you could probably still do that with an intermediate 
@rule
in the python backend. but yea.
h
Okay so is the second approach of 
FormattableTarget -> BlackTarget|Flake8Target|IsortTarget
wrong? https://github.com/pantsbuild/pants/pull/8794
w
Yes
Because you don't want to run "all rules for all formattable targets" on each target
Or else you'll end up trying to run a Java formatter on python cofe
a
in the v2 export draft PR, i have a rule converting 
TransitiveHydratedTargets
into 
ThriftableTargetCollection
it gets around the fact that we can't do 
HydratedTarget -> Optional[ThriftableTarget]
or something
w
Copy code
sources = ...
for member in union_membership.members(FormattablePythonTypes):
  sources = await Get[FormattedSources](FormattablePythonTypes, member.cls(sources))
h
Stu and I paired on this today and came up with this PR: https://github.com/pantsbuild/pants/pull/8801 It still fails, but we think is a bit closer to the solution. I posted the error in the PR. Any thoughts much appreciated on where to go from here!
a
Pushed a commit to the branch which ?maybe does the right thing?
šŸ’Æ 1
w
@hundreds-father-404: luck?
h
No, unfortunately. Danielā€™s commit fixes the rule graph ambiguity, but then results in no targets ever running with 
lint-v2
because none are satisfied with the 
union_membership.is_member(LintTarget)
. Removing that check, then we get a runtime exception
Copy code
Exception: WithDeps(Inner(InnerEntry { params: {Specs, Console, OptionsBootstrapper}, rule: Task(Task { product: Lint, clause: [Select { product: Console }, Select { product: HydratedTargets }, Select { product: UnionMembership }], gets: [Get { product: LintResults, subject: LintPythonTarget }], func: lint(), cacheable: false, display_info: Some("lint-v2") }) })) did not declare a dependency on JustGet(Get { product: LintResults, subject: PythonTargetAdaptor })
I unfortunately wonā€™t be able to finish the issue before leaving for Panama. I think Daniel may be right that we need to change the engine to allow unions within unions
w
Some wrapping is not happening there, that should be
That is a 
Get
that is claiming a particular type, but not wrapping in it
I'll see if I can look at it tonight.
ā¤ļø 1
@hundreds-father-404: just pushed a fix: https://github.com/pantsbuild/pants/pull/8801
h
šŸ˜„ thanks
Didnā€™t realize you also fixed 8801! Thank you!!!
w
Sure thing!
h
8801 also allows us to finish decoupling 
fmt
from 
lint
! Yay!
w
@hundreds-father-404: two comments: the second might kill your run
šŸ‘ 1
2 Views
Open in Slack
PreviousNext
Powered by