From 62cfe454b12f141743e666fc13a96d8e45de4314 Mon Sep 17 00:00:00 2001 From: Daniel Moran Date: Wed, 12 Apr 2023 13:38:38 -0400 Subject: [PATCH 1/3] Add tests for lint/test partitioning of fields without defaults. These tests would have previously errored out, but were fixed by #18719. Adding them now before I change the batching key logic to be sure I don't re-break anything. --- src/python/pants/core/goals/lint_test.py | 26 +++++++++++++++++++----- src/python/pants/core/goals/test_test.py | 19 ++++++++++++----- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/python/pants/core/goals/lint_test.py b/src/python/pants/core/goals/lint_test.py index 7d84a20c214..e683fd6bda8 100644 --- a/src/python/pants/core/goals/lint_test.py +++ b/src/python/pants/core/goals/lint_test.py @@ -32,7 +32,7 @@ from pants.engine.fs import PathGlobs, SpecsPaths, Workspace from pants.engine.internals.native_engine import EMPTY_SNAPSHOT, Snapshot from pants.engine.rules import QueryRule -from pants.engine.target import FieldSet, FilteredTargets, MultipleSourcesField, Target +from pants.engine.target import Field, FieldSet, FilteredTargets, MultipleSourcesField, Target from pants.engine.unions import UnionMembership from pants.option.option_types import SkipOption from pants.option.subsystem import Subsystem @@ -48,15 +48,21 @@ class MockMultipleSourcesField(MultipleSourcesField): pass +class MockRequiredField(Field): + alias = "required" + required = True + + class MockTarget(Target): alias = "mock_target" - core_fields = (MockMultipleSourcesField,) + core_fields = (MockMultipleSourcesField, MockRequiredField) @dataclass(frozen=True) class MockLinterFieldSet(FieldSet): required_fields = (MultipleSourcesField,) sources: MultipleSourcesField + required: MockRequiredField class MockLintRequest(LintRequest, metaclass=ABCMeta): @@ -307,7 +313,9 @@ def rule_runner() -> RuleRunner: def make_target(address: Optional[Address] = None) -> Target: - return MockTarget({}, address or Address("", target_name="tests")) + return MockTarget( + {MockRequiredField.alias: "present"}, address or Address("", target_name="tests") + ) def run_lint_rule( @@ -559,8 +567,16 @@ class LintKitchenRequest(LintTargetsRequest): ] rule_runner = RuleRunner(rules=rules) field_sets = ( - MockLinterFieldSet(Address("knife"), MultipleSourcesField(["knife"], Address("knife"))), - MockLinterFieldSet(Address("bowl"), MultipleSourcesField(["bowl"], Address("bowl"))), + MockLinterFieldSet( + Address("knife"), + MultipleSourcesField(["knife"], Address("knife")), + MockRequiredField("present", Address("")), + ), + MockLinterFieldSet( + Address("bowl"), + MultipleSourcesField(["bowl"], Address("bowl")), + MockRequiredField("present", Address("")), + ), ) partitions = rule_runner.request(Partitions, [LintKitchenRequest.PartitionRequest(field_sets)]) assert len(partitions) == 1 diff --git a/src/python/pants/core/goals/test_test.py b/src/python/pants/core/goals/test_test.py index bc2124e0c29..ca4c3768e9c 100644 --- a/src/python/pants/core/goals/test_test.py +++ b/src/python/pants/core/goals/test_test.py @@ -3,7 +3,7 @@ from __future__ import annotations -from abc import ABCMeta, abstractmethod +from abc import abstractmethod from dataclasses import dataclass from functools import partial from pathlib import Path @@ -66,6 +66,7 @@ ) from pants.engine.target import ( BoolField, + Field, MultipleSourcesField, Target, TargetRootsToFieldSets, @@ -123,9 +124,14 @@ class MockSkipTestsField(BoolField): default = False +class MockRequiredField(Field): + alias = "required" + required = True + + class MockTarget(Target): alias = "mock_target" - core_fields = (MockMultipleSourcesField, MockSkipTestsField) + core_fields = (MockMultipleSourcesField, MockSkipTestsField, MockRequiredField) @dataclass(frozen=True) @@ -137,8 +143,11 @@ class MockCoverageDataCollection(CoverageDataCollection): element_type = MockCoverageData -class MockTestFieldSet(TestFieldSet, metaclass=ABCMeta): - required_fields = (MultipleSourcesField,) +@dataclass(frozen=True) +class MockTestFieldSet(TestFieldSet): + required_fields = (MultipleSourcesField, MockRequiredField) + sources: MultipleSourcesField + required: MockRequiredField @classmethod def opt_out(cls, tgt: Target) -> bool: @@ -224,7 +233,7 @@ def rule_runner() -> RuleRunner: def make_target(address: Address | None = None, *, skip: bool = False) -> Target: if address is None: address = Address("", target_name="tests") - return MockTarget({MockSkipTestsField.alias: skip}, address) + return MockTarget({MockSkipTestsField.alias: skip, MockRequiredField.alias: "present"}, address) def run_test_rule( From d8753ccd84f97e6660f53edbd24d8af63652ac90 Mon Sep 17 00:00:00 2001 From: Daniel Moran Date: Wed, 12 Apr 2023 13:53:25 -0400 Subject: [PATCH 2/3] Use address as stable key when batching field sets in `lint`/`test`. Closes #18566 Using `str(x)` as the stable batching key ends up using `repr` when `x` is a field set. This previously raised an exception in some cases - it's also an unintended behavior change introduced as part of the new partitioning logic in 2.15.x. Revert back to using `.address` as the partitioning key for field sets. --- src/python/pants/core/goals/lint.py | 7 +++---- src/python/pants/core/goals/test.py | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/python/pants/core/goals/lint.py b/src/python/pants/core/goals/lint.py index 2946ca60715..bd44aa9f20e 100644 --- a/src/python/pants/core/goals/lint.py +++ b/src/python/pants/core/goals/lint.py @@ -430,12 +430,10 @@ async def lint( if not partitions_by_request_type: return Lint(exit_code=0) - def batch_by_size( - iterable: Iterable[_T], key: Callable[[_T], str] = lambda x: str(x) - ) -> Iterator[tuple[_T, ...]]: + def batch_by_size(iterable: Iterable[_T]) -> Iterator[tuple[_T, ...]]: batches = partition_sequentially( iterable, - key=key, + key=lambda x: str(x.address) if isinstance(x, FieldSet) else str(x), size_target=lint_subsystem.batch_size, size_max=4 * lint_subsystem.batch_size, ) @@ -447,6 +445,7 @@ def batch_by_size( (batch, partition.metadata) for partitions in partitions_list for partition in partitions + if partition.elements for batch in batch_by_size(partition.elements) ] for request_type, partitions_list in partitions_by_request_type.items() diff --git a/src/python/pants/core/goals/test.py b/src/python/pants/core/goals/test.py index 1dc533641d8..b24ec34e1bb 100644 --- a/src/python/pants/core/goals/test.py +++ b/src/python/pants/core/goals/test.py @@ -726,7 +726,7 @@ def partitions_get(request_type: type[TestRequest]) -> Get[Partitions]: for partition in partitions for batch in partition_sequentially( partition.elements, - key=lambda x: str(x), + key=lambda x: str(x.address) if isinstance(x, FieldSet) else str(x), size_target=test_subsystem.batch_size, size_max=2 * test_subsystem.batch_size, ) From 96fe4f775495b10da74a5fcd49ec6d9172aafe69 Mon Sep 17 00:00:00 2001 From: Daniel Moran Date: Wed, 12 Apr 2023 16:02:34 -0400 Subject: [PATCH 3/3] Remove filter --- src/python/pants/core/goals/lint.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/python/pants/core/goals/lint.py b/src/python/pants/core/goals/lint.py index bd44aa9f20e..ad9e94620e2 100644 --- a/src/python/pants/core/goals/lint.py +++ b/src/python/pants/core/goals/lint.py @@ -445,7 +445,6 @@ def batch_by_size(iterable: Iterable[_T]) -> Iterator[tuple[_T, ...]]: (batch, partition.metadata) for partitions in partitions_list for partition in partitions - if partition.elements for batch in batch_by_size(partition.elements) ] for request_type, partitions_list in partitions_by_request_type.items()