pants: The PluginField extension mechanism fails for any universe with >1 Target subclass.
The fundamental issue is here: https://github.com/pantsbuild/pants/blob/ef9e0543a80568ee270f63e1bbbeddb92c4faa08/src/python/pants/engine/target.py#L334-L339
The cls.PluginField will always be exactly Target.PluginField unless some subclass shadows the inner class member. I.E.:
$ python
Python 3.8.3 (default, May 17 2020, 18:15:42)
[GCC 10.1.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class Outer:
... class Inner:
... ...
...
>>> class SubclassNaive(Outer):
... ...
...
>>> class SubclassVerbose(Outer):
... class Inner:
... ...
...
>>> Outer.Inner is SubclassNaive.Inner
True
>>> Outer.Inner is SubclassVerbose.Inner
False
>>>
The effect of which is probably best shown with a few tests:
$ git diff src/python/pants/engine/target_test.py
diff --git a/src/python/pants/engine/target_test.py b/src/python/pants/engine/target_test.py
index f2646d33c..16a3a9060 100644
--- a/src/python/pants/engine/target_test.py
+++ b/src/python/pants/engine/target_test.py
@@ -13,7 +13,7 @@ from pants.base.specs import FilesystemLiteralSpec
from pants.engine.addresses import Address, Addresses
from pants.engine.fs import EMPTY_DIGEST, FileContent, InputFilesContent, PathGlobs, Snapshot
from pants.engine.internals.scheduler import ExecutionError
-from pants.engine.rules import RootRule, rule
+from pants.engine.rules import RootRule, RuleIndex, rule
from pants.engine.selectors import Get, Params
from pants.engine.target import (
AmbiguousCodegenImplementationsException,
@@ -270,35 +270,75 @@ def test_async_field() -> None:
assert "//:lib" in str(exc)
-def test_add_custom_fields() -> None:
- class CustomField(BoolField):
- alias: ClassVar = "custom_field"
- default: ClassVar = False
+class RustTarget(Target):
+ alias: ClassVar = "rust"
+ core_fields: ClassVar = (Dependencies, Sources)
+
+
+class RustEditionCustomField(StringField):
+ alias: ClassVar = "rust_edition"
+ required: ClassVar = True
+
+
+class FortranCustomField(BoolField):
+ alias: ClassVar = "custom_field"
+ default: ClassVar = False
- union_membership = UnionMembership({FortranTarget.PluginField: [CustomField]})
- tgt_values = {CustomField.alias: True}
+
+def assert_fortran_target_fields(union_membership: UnionMembership) -> None:
+ assert {FortranExtensions, FortranSources, FortranCustomField} == set(
+ FortranTarget.class_field_types(union_membership)
+ )
+
+
+def test_add_custom_fields_last_wins_bug() -> None:
+ union_membership = UnionMembership(
+ {
+ FortranTarget.PluginField: [FortranCustomField],
+ RustTarget.PluginField: [RustEditionCustomField],
+ }
+ )
+ assert_fortran_target_fields(union_membership)
+
+
+def test_add_custom_fields_full_union_bug() -> None:
+ rule_index = RuleIndex.create(
+ [
+ UnionRule(FortranTarget.PluginField, FortranCustomField),
+ UnionRule(RustTarget.PluginField, RustEditionCustomField),
+ ]
+ )
+ union_membership = UnionMembership(rule_index.normalized_rules().union_rules)
+ assert_fortran_target_fields(union_membership)
+
+
+def test_add_custom_fields() -> None:
+ union_membership = UnionMembership({FortranTarget.PluginField: [FortranCustomField]})
+ tgt_values = {FortranCustomField.alias: True}
tgt = FortranTarget(
tgt_values, address=Address.parse(":lib"), union_membership=union_membership
)
- assert tgt.field_types == (FortranExtensions, FortranSources, CustomField)
+ assert tgt.field_types == (FortranExtensions, FortranSources, FortranCustomField)
assert tgt.core_fields == (FortranExtensions, FortranSources)
- assert tgt.plugin_fields == (CustomField,)
- assert tgt.has_field(CustomField) is True
+ assert tgt.plugin_fields == (FortranCustomField,)
+ assert tgt.has_field(FortranCustomField) is True
assert FortranTarget.class_field_types(union_membership=union_membership) == (
FortranExtensions,
FortranSources,
- CustomField,
+ FortranCustomField,
+ )
+ assert (
+ FortranTarget.class_has_field(FortranCustomField, union_membership=union_membership) is True
)
- assert FortranTarget.class_has_field(CustomField, union_membership=union_membership) is True
- assert tgt[CustomField].value is True
+ assert tgt[FortranCustomField].value is True
default_tgt = FortranTarget(
{}, address=Address.parse(":default"), union_membership=union_membership
)
- assert default_tgt[CustomField].value is False
+ assert default_tgt[FortranCustomField].value is False
def test_override_preexisting_field_via_new_target() -> None:
Which reveals:
$ NO_REGEN_PEX=true ./v2 test src/python/pants/engine/target_test.py --pytest-args=-vvktest_add_custom_fields_bug_
01:37:06:659 [INFO] Starting tests: src/python/pants/engine:tests
01:37:11:732 [INFO] Tests failed: src/python/pants/engine:tests
𐄂 src/python/pants/engine:tests
============================= test session starts ==============================
platform linux -- Python 3.8.3, pytest-5.3.5, py-1.8.1, pluggy-0.13.1 -- /home/jsirois/dev/pantsbuild/jsirois-pants/build-support/virtualenvs/Linux/pants_dev_deps.py38.venv/bin/python
cachedir: .pytest_cache
rootdir: /tmp/process-executionfueO7a
plugins: cov-2.8.1, icdiff-0.5, timeout-1.3.4
collecting ... collected 32 items / 30 deselected / 2 selected
pants/engine/target_test.py::test_add_custom_fields_bug_last_wins FAILED [ 50%]
pants/engine/target_test.py::test_add_custom_fields_bug_full_union FAILED [100%]
=================================== FAILURES ===================================
_____________________ test_add_custom_fields_bug_last_wins _____________________
def test_add_custom_fields_bug_last_wins() -> None:
union_membership = UnionMembership(
{
FortranTarget.PluginField: [FortranCustomField],
RustTarget.PluginField: [RustEditionCustomField],
}
)
> assert_fortran_target_fields(union_membership)
pants/engine/target_test.py:301:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
union_membership = UnionMembership(union_rules=FrozenDict({<class 'pants.util.meta.PluginField'>: FrozenOrderedSet([<class 'pants.engine.target_test.RustEditionCustomField'>])}))
def assert_fortran_target_fields(union_membership: UnionMembership) -> None:
> assert {FortranExtensions, FortranSources, FortranCustomField} == set(
FortranTarget.class_field_types(union_membership)
)
E AssertionError: assert equals failed
E set([ set([
E <class 'pants.engine.target_t <class 'pants.engine.target_t
E est.FortranExtensions'>, est.FortranExtensions'>,
E <class 'pants.engine.target_t <class 'pants.engine.target_t
E est.FortranCustomField'>, est.RustEditionCustomField'>,
E <class 'pants.engine.target_t <class 'pants.engine.target_t
E est.FortranSources'>, est.FortranSources'>,
E ]) ])
pants/engine/target_test.py:289: AssertionError
____________________ test_add_custom_fields_bug_full_union _____________________
def test_add_custom_fields_bug_full_union() -> None:
rule_index = RuleIndex.create(
[
UnionRule(FortranTarget.PluginField, FortranCustomField),
UnionRule(RustTarget.PluginField, RustEditionCustomField),
]
)
union_membership = UnionMembership(rule_index.normalized_rules().union_rules)
> assert_fortran_target_fields(union_membership)
pants/engine/target_test.py:312:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
union_membership = UnionMembership(union_rules=FrozenDict({<class 'pants.util.meta.PluginField'>: FrozenOrderedSet([<class 'pants.engine.target_test.FortranCustomField'>, <class 'pants.engine.target_test.RustEditionCustomField'>])}))
def assert_fortran_target_fields(union_membership: UnionMembership) -> None:
> assert {FortranExtensions, FortranSources, FortranCustomField} == set(
FortranTarget.class_field_types(union_membership)
)
E AssertionError: assert equals failed
E set([ set([
E <class 'pants.engine.target_t <class 'pants.engine.target_t
E est.FortranExtensions'>, est.FortranExtensions'>,
E <class 'pants.engine.target_t
E est.RustEditionCustomField'>,
E <class 'pants.engine.target_t <class 'pants.engine.target_t
E est.FortranCustomField'>, est.FortranCustomField'>,
E <class 'pants.engine.target_t <class 'pants.engine.target_t
E est.FortranSources'>, est.FortranSources'>,
E ]) ])
pants/engine/target_test.py:289: AssertionError
======================= 2 failed, 30 deselected in 1.19s =======================
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 21 (21 by maintainers)
#7654 is the key ticket that was mentioned in #9826 as something we should try to fix before
1.30.x. So the general idea (if not this instance) is pretty immediate.I don’t think that the ability to dynamically add/remove rules is in the cards, no. We will likely continue to need to know about all of them before constructing the scheduler.
If it works for tests (
TestBase), where multiple schedulers are used within the same process (although not concurrently: test method at a time), then it might be ok. But the mutability ofBuildConfigInitializerandOptionsInitializerstill bites us periodically in unit tests. It feels like a pretty significant increase in usability/understandability would be needed to justify the magic involved.Yea. And I’m not sure that that precise usecase is even desirable. But many other globals/singletons (that were more likely to change run-over-run) have stood between us and concurrent runs of pants within one process, and we’ve made good progress on removing them.
I’m slightly negative on new entrypoints as Eric said, as I think it would be preferable to adapt the usecases to avoid (or use more gently) subclassing rather than add new facilities. But not worth blocking over.
This I’m more skeptical of.
__subclasses__is a global mutable singleton, which we have otherwise made great progress on removing from the codebase. They’re harder to test, more magical, make it impossible to share a process with another run, etc.