https://pantsbuild.org/ logo
#development
Title
# development
a

average-vr-56795

11/28/2019, 4:56 PM
@hundreds-father-404 Do you have a handle on https://github.com/pantsbuild/pants/issues/8733 or looking for a hand?
w

witty-crayon-22786

12/03/2019, 4:30 AM
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

hundreds-father-404

12/03/2019, 4:44 AM
And will this need to be used for goals like test too, once we introduce junit?
w

witty-crayon-22786

12/03/2019, 4:44 AM
No: see other response in the channel
👍 1
a

aloof-angle-91616

12/03/2019, 6:45 AM
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

hundreds-father-404

12/11/2019, 8:14 PM
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

witty-crayon-22786

12/11/2019, 8:22 PM
second paragraph looks right.
h

hundreds-father-404

12/11/2019, 8:23 PM
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

aloof-angle-91616

12/11/2019, 10:18 PM
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

hundreds-father-404

12/11/2019, 10:19 PM
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

aloof-angle-91616

12/11/2019, 10:19 PM
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

hundreds-father-404

12/11/2019, 10:20 PM
looking at the code
let me post my WIP! https://github.com/pantsbuild/pants/pull/8794
a

aloof-angle-91616

12/11/2019, 10:21 PM
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

witty-crayon-22786

12/11/2019, 10:22 PM
@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

aloof-angle-91616

12/11/2019, 10:22 PM
oh yeah
w

witty-crayon-22786

12/11/2019, 10:23 PM
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

hundreds-father-404

12/11/2019, 10:23 PM
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

witty-crayon-22786

12/11/2019, 10:23 PM
Yes
Which part?
h

hundreds-father-404

12/11/2019, 10:25 PM
The proposal still involves creating
IsortFormattableTarget
and
BlackFormattableTarget
, right? Are those directly members of the
TargetWithSources
union? There’s no intermediate type
PythonFormattableTarget
?
w

witty-crayon-22786

12/11/2019, 10:25 PM
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

hundreds-father-404

12/11/2019, 10:53 PM
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

witty-crayon-22786

12/11/2019, 10:54 PM
this is a new union that is not "TargetWithSources"
h

hundreds-father-404

12/11/2019, 10:55 PM
this is a new union that is not “TargetWithSources”
Why is that / does
TargetWithSources
have any role still or that should be deleted?
w

witty-crayon-22786

12/11/2019, 10:55 PM
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

hundreds-father-404

12/11/2019, 10:57 PM
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

witty-crayon-22786

12/11/2019, 10:58 PM
this would need to be in the python backend somewhere, i think.
h

hundreds-father-404

12/11/2019, 10:59 PM
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

witty-crayon-22786

12/11/2019, 11:00 PM
"all targets with sources" are not necessarily formattable, so that formulation of TargetWithSources is confusing to me.
h

hundreds-father-404

12/11/2019, 11:00 PM
Agreed. It would be more clear to have
FormattableTarget
and then
PythonFormattableTarget
w

witty-crayon-22786

12/11/2019, 11:01 PM
yea.
h

hundreds-father-404

12/11/2019, 11:01 PM
This was my first experiment. Go from
FormattableTarget -> PythonFormattableTarget -> BlackTarget|Flake8Target|IsortTarget
, but it failed because we can’t have nested unions
w

witty-crayon-22786

12/11/2019, 11:02 PM
... 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

hundreds-father-404

12/11/2019, 11:04 PM
Okay so is the second approach of
FormattableTarget -> BlackTarget|Flake8Target|IsortTarget
wrong? https://github.com/pantsbuild/pants/pull/8794
w

witty-crayon-22786

12/11/2019, 11:06 PM
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

aloof-angle-91616

12/11/2019, 11:38 PM
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

witty-crayon-22786

12/12/2019, 2:05 AM
Copy code
sources = ...
for member in union_membership.members(FormattablePythonTypes):
  sources = await Get[FormattedSources](FormattablePythonTypes, member.cls(sources))
h

hundreds-father-404

12/12/2019, 6:09 AM
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

average-vr-56795

12/12/2019, 11:51 AM
Pushed a commit to the branch which ?maybe does the right thing?
💯 1
w

witty-crayon-22786

12/12/2019, 10:09 PM
@hundreds-father-404: luck?
h

hundreds-father-404

12/12/2019, 10:13 PM
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

witty-crayon-22786

12/12/2019, 10:14 PM
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

hundreds-father-404

12/13/2019, 4:33 AM
😄 thanks
Didn’t realize you also fixed 8801! Thank you!!!
w

witty-crayon-22786

12/13/2019, 4:41 AM
Sure thing!
h

hundreds-father-404

12/13/2019, 5:50 AM
8801 also allows us to finish decoupling
fmt
from
lint
! Yay!
w

witty-crayon-22786

12/13/2019, 5:28 PM
@hundreds-father-404: two comments: the second might kill your run
👍 1