Skip to content

Commit 200447e

Browse files
mohamedzeidan2021Mohamed Zeidan
andauthored
fix: make all pipeline Parameters hashable (#4601) (#6351)
The base Parameter is an attrs class; attrs sets __hash__ = None for eq-enabled non-frozen classes, so ParameterInteger, ParameterFloat and ParameterBoolean were unhashable. Only ParameterString worked, and its hash(tuple(self.to_request())) implementation was itself buggy (it hashed the request dict's keys, not its values, violating the eq/hash invariant). Decorate the base with @attr.s(hash=False) so attrs keeps a body-defined __hash__ while still generating __eq__, and add a base __hash__ keyed on (name, parameter_type.value) - a subset of the equality fields, preserving the a == b implies hash(a) == hash(b) invariant. Remove the redundant ParameterString.__hash__. Co-authored-by: Mohamed Zeidan <zeidmo@amazon.com>
1 parent c2721f0 commit 200447e

2 files changed

Lines changed: 127 additions & 5 deletions

File tree

‎sagemaker-core/src/sagemaker/core/workflow/parameters.py‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ def python_type(self) -> Type:
5151
return mapping[self]
5252

5353

54-
@attr.s
54+
# ``hash=False`` prevents attrs from clobbering the ``__hash__`` defined in the class body
55+
# with ``None`` (its default behavior for an eq-enabled, non-frozen class). ``__eq__`` is
56+
# still generated. This keeps all Parameter subtypes hashable, not just ``ParameterString``.
57+
@attr.s(hash=False)
5558
class Parameter(PipelineVariable, Entity):
5659
"""Pipeline parameter for workflow.
5760
@@ -65,6 +68,16 @@ class Parameter(PipelineVariable, Entity):
6568
parameter_type: ParameterTypeEnum = attr.ib(factory=ParameterTypeEnum.factory)
6669
default_value: PrimitiveType = attr.ib(default=None)
6770

71+
def __hash__(self):
72+
"""Hash a parameter on its name and type.
73+
74+
Keys on a subset of the fields used for equality so that the
75+
``a == b`` implies ``hash(a) == hash(b)`` invariant always holds.
76+
Do not mutate ``name`` after using an instance as a set member or
77+
dict key, as that strands the entry (standard Python hashing hazard).
78+
"""
79+
return hash((self.name, self.parameter_type.value))
80+
6881
@default_value.validator
6982
def _check_default_value(self, _, value):
7083
"""Check whether the default value is compatible with the parameter type.
@@ -168,10 +181,6 @@ def __init__(self, name: str, default_value: str = None, enum_values: List[str]
168181
)
169182
self.enum_values = enum_values
170183

171-
def __hash__(self):
172-
"""Hash function for parameter types"""
173-
return hash(tuple(self.to_request()))
174-
175184
def to_string(self) -> PipelineVariable:
176185
"""Prompt the pipeline to convert the pipeline variable to String in runtime
177186
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You
4+
# may not use this file except in compliance with the License. A copy of
5+
# the License is located at
6+
#
7+
# http://aws.amazon.com/apache2.0/
8+
#
9+
# or in the "license" file accompanying this file. This file is
10+
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
11+
# ANY KIND, either express or implied. See the License for the specific
12+
# language governing permissions and limitations under the License.
13+
"""Unit tests for pipeline parameters, focusing on hashability."""
14+
15+
from __future__ import absolute_import
16+
17+
import pytest
18+
19+
from sagemaker.core.workflow.parameters import (
20+
Parameter,
21+
ParameterBoolean,
22+
ParameterFloat,
23+
ParameterInteger,
24+
ParameterString,
25+
ParameterTypeEnum,
26+
)
27+
28+
29+
@pytest.fixture
30+
def all_parameters():
31+
"""Return one instance of each concrete parameter type."""
32+
return [
33+
ParameterString(name="MyString", default_value="foo"),
34+
ParameterInteger(name="MyInteger", default_value=1),
35+
ParameterFloat(name="MyFloat", default_value=1.0),
36+
ParameterBoolean(name="MyBoolean", default_value=True),
37+
]
38+
39+
40+
def test_all_parameters_are_hashable(all_parameters):
41+
"""Every concrete parameter type must be hashable."""
42+
for param in all_parameters:
43+
assert isinstance(hash(param), int)
44+
45+
46+
def test_base_parameter_is_hashable():
47+
"""The base Parameter must be hashable."""
48+
param = Parameter(name="Base", parameter_type=ParameterTypeEnum.STRING)
49+
assert isinstance(hash(param), int)
50+
51+
52+
def test_parameters_usable_as_set_members(all_parameters):
53+
"""Parameters must be usable as set members without raising."""
54+
param_set = set(all_parameters)
55+
assert len(param_set) == len(all_parameters)
56+
for param in all_parameters:
57+
assert param in param_set
58+
59+
60+
def test_parameters_usable_as_dict_keys(all_parameters):
61+
"""Parameters must be usable as dict keys without raising."""
62+
mapping = {param: param.name for param in all_parameters}
63+
for param in all_parameters:
64+
assert mapping[param] == param.name
65+
66+
67+
@pytest.mark.parametrize(
68+
"factory, kwargs",
69+
[
70+
(ParameterString, {"name": "P", "default_value": "v"}),
71+
(ParameterInteger, {"name": "P", "default_value": 3}),
72+
(ParameterFloat, {"name": "P", "default_value": 3.0}),
73+
(ParameterBoolean, {"name": "P", "default_value": False}),
74+
],
75+
)
76+
def test_equal_parameters_hash_equal(factory, kwargs):
77+
"""Equal parameters must hash to the same value (eq/hash invariant)."""
78+
first = factory(**kwargs)
79+
second = factory(**kwargs)
80+
assert first == second
81+
assert hash(first) == hash(second)
82+
83+
84+
def test_different_type_parameters_are_distinct():
85+
"""Parameters of the same name but different type are unequal and distinct.
86+
87+
Note hash *inequality* is not part of the hashing contract (collisions are
88+
legal), so we assert on equality/distinct-membership rather than hashes.
89+
"""
90+
integer_param = ParameterInteger(name="Shared")
91+
float_param = ParameterFloat(name="Shared")
92+
assert integer_param != float_param
93+
assert len({integer_param, float_param}) == 2
94+
95+
96+
def test_parameter_string_dedupes_equal_instances():
97+
"""ParameterString remains hashable and dedupes equal instances in a set."""
98+
first = ParameterString(name="Dup", default_value="x")
99+
second = ParameterString(name="Dup", default_value="x")
100+
assert len({first, second}) == 1
101+
102+
103+
def test_parameter_string_hash_invariant_ignores_enum_values():
104+
"""Equal ParameterStrings that differ only in enum_values must hash equal.
105+
106+
``enum_values`` is not part of attrs equality, so two such instances are
107+
equal; the old ``hash(tuple(self.to_request()))`` implementation broke the
108+
``a == b`` implies ``hash(a) == hash(b)`` invariant here.
109+
"""
110+
with_enum = ParameterString(name="P", enum_values=["a", "b"])
111+
without_enum = ParameterString(name="P")
112+
assert with_enum == without_enum
113+
assert hash(with_enum) == hash(without_enum)

0 commit comments

Comments
 (0)