Skip to content

Commit

Permalink
Remove overriden parameters from address.
Browse files Browse the repository at this point in the history
This fix a bug where default parametrize resolve with overrides would
have inconsistent parameters and field values.
  • Loading branch information
isra17 committed May 22, 2024
1 parent 40e8bc9 commit ea39d3e
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 24 deletions.
4 changes: 4 additions & 0 deletions docs/notes/2.22.x.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ If you encounter such discrepancies, and you can't resolve them easily, please [

### Backends

#### BUILD

Non-parametrized keys in `overrides` or parameter group will now be removed from the target parameters. This fix [a bug](https://github.com/pantsbuild/pants/issues/20933) where a target with parametrized default would have inconsistent parameters with its field values.

#### NEW: SQL

A new experimental `SQL` backend was added along with the [sqlfluff
Expand Down
5 changes: 3 additions & 2 deletions src/python/pants/engine/internals/native_engine.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,9 @@ class Address:
...
@property
def path_safe_spec(self) -> str: ...
def parametrize(self, parameters: Mapping[str, str]) -> Address:
"""Creates a new Address with the given `parameters` merged over self.parameters."""
def parametrize(self, parameters: Mapping[str, str], replace: bool = False) -> Address:
"""Creates a new Address with the given `parameters` merged or replaced over
self.parameters."""
...
def maybe_convert_to_target_generator(self) -> Address:
"""If this address is generated or parametrized, convert it to its generator target.
Expand Down
50 changes: 31 additions & 19 deletions src/python/pants/engine/internals/parametrize.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,42 +166,54 @@ def expand(
except Exception as e:
raise Exception(f"Failed to parametrize `{address}`:\n {e}") from e

parameters = address.parameters
non_parametrized = tuple(
(field_name, field_value)
for field_name, field_value in fields.items()
if not isinstance(field_value, Parametrize)
)
non_parametrized_keys = {k for k, _ in non_parametrized}

# Remove non-parametrized fields from the address parameters.
for k in non_parametrized_keys & parameters.keys():
parameters.pop(k, None)

if parametrized_groups:
# Add the groups as one vector for the cross-product.
parametrized.append(parametrized_groups)

if not parametrized:
address = address.parametrize(parameters, replace=True)
yield (address, fields)
return

non_parametrized = tuple(
(field_name, field_value)
for field_name, field_value in fields.items()
if not isinstance(field_value, Parametrize)
)

for parametrized_args in itertools.product(*parametrized):
expanded_address = address.parametrize(
{field_name: alias for field_name, alias, _ in parametrized_args}
expanded_parameters = parameters | {
field_name: alias for field_name, alias, _ in parametrized_args
}
# There will be at most one group per cross product.
group_kwargs: Mapping[str, Any] = next(
(
field_value.kwargs
for _, _, field_value in parametrized_args
if isinstance(field_value, Parametrize) and field_value.is_group
),
{},
)
# Exclude group parameter from address parameters
for k in group_kwargs.keys() & parameters.keys():
expanded_parameters.pop(k, None)

parametrized_args_fields = tuple(
(field_name, field_value)
for field_name, _, field_value in parametrized_args
# Exclude any parametrize group
if not (isinstance(field_value, Parametrize) and field_value.is_group)
)
expanded_fields: dict[str, Any] = dict(non_parametrized + parametrized_args_fields)
expanded_fields.update(
# There will be at most one group per cross product.
next(
(
field_value.kwargs
for _, _, field_value in parametrized_args
if isinstance(field_value, Parametrize) and field_value.is_group
),
{},
)
)
expanded_fields.update(group_kwargs)

expanded_address = address.parametrize(expanded_parameters, replace=True)
yield expanded_address, expanded_fields

@staticmethod
Expand Down
53 changes: 53 additions & 0 deletions src/python/pants/engine/internals/parametrize_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,59 @@ def test_expand(
)


@pytest.mark.parametrize(
"expected,parameters,fields",
[
# Single parameters remove any existing parameters from the address.
([("a:a", {"f": "1"})], {"f": "0"}, {"f": "1"}),
# But keeps other parameters.
([("a@g=0", {"f": "1"})], {"g": "0"}, {"f": "1"}),
# Test both case at the same time.
([("a@g=0", {"f": "1"})], {"f": "0", "g": "0"}, {"f": "1"}),
# Group parameters remove existing covered parameters.
(
[
("a@h=0,parametrize=A", {"f": "1", "g": "2", "i": "1"}),
("a@f=0,h=0,parametrize=B", {"g": "1", "i": "1"}),
],
{"f": "0", "g": "0", "h": "0", "i": "0"},
dict(
g="1",
i="1",
**Parametrize("A", f="1", g="2"), # type: ignore[arg-type]
**Parametrize("B"),
),
),
# Re-Parametrize existing parameters
(
[
("a@f=1,g=1,h=0", {"f": "1", "g": "1"}),
("a@f=1,g=2,h=0", {"f": "1", "g": "2"}),
],
{"f": "0", "g": "0", "h": "0"},
{
"f": Parametrize("1"),
"g": Parametrize("1", "2"),
},
),
],
)
def test_expand_existing_parameters(
expected: list[tuple[str, dict[str, Any]]],
parameters: dict[str, Any],
fields: dict[str, Any | Parametrize],
) -> None:
assert sorted(expected) == sorted(
(
(address.spec, result_fields)
for address, result_fields in Parametrize.expand(
Address("a", parameters=parameters), fields
)
),
key=lambda value: value[0],
)


@pytest.mark.parametrize(
"fields, expected_error",
[
Expand Down
11 changes: 8 additions & 3 deletions src/rust/engine/src/externs/address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -680,9 +680,14 @@ impl Address {
Ok(format!("{prefix}{path}{target}{generated}{params}"))
}

fn parametrize(&self, parameters: BTreeMap<String, String>) -> Self {
let mut merged_parameters = self.parameters.clone();
merged_parameters.extend(parameters);
fn parametrize(&self, parameters: BTreeMap<String, String>, replace: bool) -> Self {
let merged_parameters = if replace {
parameters
} else {
let mut merged_parameters = parameters.clone();
merged_parameters.extend(parameters);
merged_parameters
};

Self {
spec_path: self.spec_path.clone(),
Expand Down

0 comments on commit ea39d3e

Please sign in to comment.