union question :thread:
# development
union question ๐Ÿงต
From `unions.py`:
Subclassed union bases should inherit the superclass's union members.
Copy code
class Base: ...

class Sub(Base): ...

class BaseInput(Base): ...
class SubInput(Sub): ...
class Result: ...

# ...

UnionRule(Sub, SubInput)
UnionRule(Base, BaseInput)

# ...

await Get(Result, Sub(...))
is complaining:
Copy code
No installed rules return the type Result, and it was not provided by potential callers of Query(Result for BaseInput).
Sure enough, if I print the membership of
I see
<class BaseInput>
I guess I have to invert my type hierarchy. Seems goofy tho ๐Ÿค”
Hm inheritance doesn't actually matter, that's only convention
In this case, we're talking about union inheritance.
So, at a minimum I think the comment has a typo/misnomer. It should read:
Union subclasses of union base classes should inherit the base class's members
๐Ÿ‘ 1
Or don't talk about inheritence. The engine does not care about inheritance
In this case, it does care about inheritance ๐Ÿ‘€
What I want though is the reverse. I want a shared base that I can query and get all of its registered members AND the subclass registered unions. I could invert the type hierarchy, but then my type hints are all backwards as well and the code gets confusing
Actually I think this code has a bug anyways...
Copy code
for sub_union in union_base.__subclasses__():
should be:
Copy code
for sub_union in union_base.__subclasses__():
    if is_union(sub_union):
The result right now is the union membership has a lot of mapping from our union subclasses (E.g.
) back to the parent (e.g.
) members (so like every formatter...)
Copy code
Copy code
09:56:48.25 [INFO] stdout: "FrozenOrderedSet([<class 'pants.backend.python.lint.autoflake.rules.AutoflakeRequest'>, <class 'pants.backend.python.lint.black.rules.BlackRequest'>, <class 'pants.backend.python.lint.docformatter.rules.DocformatterRequest'>, <class 'pants.backend.python.lint.isort.rules.IsortRequest'>, <class 'pants.backend.shell.lint.shfmt.rules.ShfmtRequest'>, <class 'pants.backend.go.lint.gofmt.rules.GofmtRequest'>, <class 'pants.backend.java.lint.google_java_format.rules.GoogleJavaFormatRequest'>, <class 'pants.backend.scala.lint.scalafmt.rules.ScalafmtRequest'>])"
womp womp, lol
And the good news, if we fix this bug, there are no sub unions today in our codebase. I'm going to suggest we invert the relationship. PR will be coming soonish
Funny enough, it's our convention that is causing this ๐Ÿ˜… Very tiny (likely not noticeable) performance improvement incoming ๐Ÿ˜›
๐Ÿ‘ 1
โค๏ธ 1
Copy code
$ git diff main
diff --git a/src/python/pants/engine/unions_test.py b/src/python/pants/engine/unions_test.py
index f302f180e..40b7d7f32 100644
--- a/src/python/pants/engine/unions_test.py
+++ b/src/python/pants/engine/unions_test.py
@@ -10,10 +10,10 @@ def test_union_membership_from_rules() -> None:
     class Base:
-    class A:
+    class A(Base):
-    class B:
+    class B(Base):
     assert UnionMembership.from_rules([UnionRule(Base, A), UnionRule(Base, B)]) == UnionMembership(
^ That fails because we also map
and vice-versa. Proof of fix:
Copy code
$ git diff main
diff --git a/src/python/pants/engine/unions.py b/src/python/pants/engine/unions.py
index 13dd10c75..47389c956 100644
--- a/src/python/pants/engine/unions.py
+++ b/src/python/pants/engine/unions.py
@@ -77,9 +77,10 @@ class UnionMembership:
         while len(bases) > 0:
             union_base = bases.pop()
             for sub_union in union_base.__subclasses__():
-                if sub_union not in mapping:
-                    bases.append(sub_union)
-                mapping[sub_union].update(mapping[union_base])
+                if is_union(sub_union):
+                    if sub_union not in mapping:
+                        bases.append(sub_union)
+                    mapping[sub_union].update(mapping[union_base])
         return cls(mapping)
     def __init__(self, union_rules: Mapping[type, Iterable[type]]) -> None:
Now the test passes ๐Ÿ˜›
is waaaaaaaaaaaaaaaaaaaaaaaaay too small for the complexity
๐Ÿ’ฏ 1
โž• 1
@curved-television-6568 you implemented the code (and didn't update tests, tsk tsk) https://github.com/pantsbuild/pants/commit/643d8da78b6fbd0e57fcd94dd82e1077106f3d68#diff-517fa581d812162b8b4cf7e16[โ€ฆ]4613c961f83f74daa05a58855706fe6 From the PR:
Otherwise union.get(SomeTarget.PluginFields) wouldn't give you anything when you register a plugin field on Target.
I don't quite follow ๐Ÿ˜›
Ah OK I think I see. I'll need to invert or change
with the new behavior I think
Although, I think this does break the model for the Target code. Didn't know plugins were used here. Gonna have think on this one ๐Ÿ™‚
you implemented the code (and didnโ€™t update tests, tsk tsk)
waddaโ€™ya mean? I see I didnโ€™t add any unit tests for the union directly, but the change is tested indirectly by the target tests.. ๐Ÿ™‚
See my diff above. The convention we have of subclassing unions now has
mapping non-union subclasses ๐Ÿ˜‰
doh ๐Ÿ˜›
After a lot of headscratching I'll update the code to invert the mapping, but not break your new stuff
๐Ÿ‘ 1
โค๏ธ 1
So I think based on this use-case the code makes sense, and my use-case needs to flex a little bit. BUT I also think the code needs some TLC. I'll do both
โค๏ธ 1
When that is submitted I have a follow-up PR for my use-case?