From 8159889267fc5aca2bede3062f8d4b2d6590a2d1 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Fri, 28 Aug 2026 22:58:12 +0530 Subject: [PATCH 1/2] [BUG] roll back state when set_params fails validation set_params writes parameter values to self via setattr before calling reset(), which is what re-runs __init__ to validate them. If __init__ rejects a value, the raise happens after the write, so the rejected value survives on self -- a state __init__ could never have produced. get_params, clone, and later set_params calls then operate on that invalid state. Snapshot self.__dict__ before the write loop in set_params and restore it if the loop or the following reset() call raises. Adds a regression test with a fixture that actually validates in __init__, since the existing test_get_params_after_set_params never exercised the raise path (its fixture class never raises). Fixes sktime/sktime#10695 --- skbase/base/_base.py | 50 ++++++++++++++++++++++++--------------- skbase/tests/test_base.py | 42 ++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 19 deletions(-) diff --git a/skbase/base/_base.py b/skbase/base/_base.py index 173c7fd9..e3f113d7 100644 --- a/skbase/base/_base.py +++ b/skbase/base/_base.py @@ -384,28 +384,40 @@ def set_params(self, **params): return self valid_params = self.get_params(deep=True) + # snapshot instance state, to restore it if setting params below raises, + # e.g., through a validation failure in __init__ during the reset call. + # Without this, a failed set_params can leave self in a state that + # __init__ could not have produced, since setattr writes below happen + # before that validation runs. + prev_state = self.__dict__.copy() + unmatched_keys = [] nested_params = defaultdict(dict) # grouped by prefix - for full_key, value in params.items(): - # split full_key by first occurrence of __, if contains __ - # "key_without_dblunderscore" -> "key_without_dbl_underscore", None, None - # "key__with__dblunderscore" -> "key", "__", "with__dblunderscore" - key, delim, sub_key = full_key.partition("__") - # if key not recognized, remember for suffix matching - if key not in valid_params: - unmatched_keys += [key] - # if full_key contained __, collect suffix for component set_params - elif delim: - nested_params[key][sub_key] = value - # if key is found and did not contain __, set self.key to the value - else: - setattr(self, key, value) - valid_params[key] = value - - # all matched params have now been set - # reset object to clean post-init state with those params - self.reset() + try: + for full_key, value in params.items(): + # split full_key by first occurrence of __, if contains __ + # "key_without_dblunderscore" -> "key_without_dbl_underscore", None, None + # "key__with__dblunderscore" -> "key", "__", "with__dblunderscore" + key, delim, sub_key = full_key.partition("__") + # if key not recognized, remember for suffix matching + if key not in valid_params: + unmatched_keys += [key] + # if full_key contained __, collect suffix for component set_params + elif delim: + nested_params[key][sub_key] = value + # if key is found and did not contain __, set self.key to the value + else: + setattr(self, key, value) + valid_params[key] = value + + # all matched params have now been set + # reset object to clean post-init state with those params + self.reset() + except Exception: + self.__dict__.clear() + self.__dict__.update(prev_state) + raise # recurse in components for key, sub_params in nested_params.items(): diff --git a/skbase/tests/test_base.py b/skbase/tests/test_base.py index 33e48aec..d4cbdd58 100644 --- a/skbase/tests/test_base.py +++ b/skbase/tests/test_base.py @@ -903,6 +903,48 @@ def test_set_params_with_no_param_to_set_returns_object( ) +class ValidatingObject(BaseObject): + """BaseObject whose __init__ writes then validates a parameter. + + Regression fixture for https://github.com/sktime/sktime/issues/10695 : + __init__ assigns ``self.x`` before raising, mirroring the common pattern + of estimators that write a hyper-parameter to ``self`` ahead of a + validation check. + """ + + def __init__(self, x=1): + self.x = x + if x < 0: + raise ValueError("x must be non-negative") + super().__init__() + + +def test_set_params_rolls_back_state_on_invalid_value(): + """Test a failed set_params leaves the object in its pre-call state. + + A failed set_params previously left self holding the rejected value, + since set_params writes parameters to self before reset() re-runs + __init__ for validation. A state __init__ could never have produced + would then survive the raise, and break get_params, clone and repeated + set_params calls from that point on. + """ + obj = ValidatingObject(x=5) + + with pytest.raises(ValueError, match="x must be non-negative"): + obj.set_params(x=-1) + + assert obj.x == 5 + assert obj.get_params() == {"x": 5} + + # clone should still work off the pre-call state + cloned = obj.clone() + assert cloned.get_params() == {"x": 5} + + # a subsequent valid set_params call should still work normally + obj.set_params(x=9) + assert obj.get_params() == {"x": 9} + + # This section tests the clone functionality # These have been adapted from sklearn's tests of clone to use the clone # method that is included as part of the BaseObject interface From f014d40245a6a291740a6c1c506e1576f1499039 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Sun, 30 Aug 2026 16:43:51 +0530 Subject: [PATCH 2/2] [BUG] narrow rollback to reset, raise explicit error on failed set_params --- skbase/base/_base.py | 58 ++++++++++++++++++++++----------------- skbase/tests/test_base.py | 8 +++++- 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/skbase/base/_base.py b/skbase/base/_base.py index e3f113d7..926b0158 100644 --- a/skbase/base/_base.py +++ b/skbase/base/_base.py @@ -384,40 +384,48 @@ def set_params(self, **params): return self valid_params = self.get_params(deep=True) - # snapshot instance state, to restore it if setting params below raises, - # e.g., through a validation failure in __init__ during the reset call. - # Without this, a failed set_params can leave self in a state that - # __init__ could not have produced, since setattr writes below happen - # before that validation runs. + # snapshot instance state, to restore it if the reset call below raises, + # e.g., through a parameter validation failure in __init__. + # Must be taken before the setattr writes below, since those writes are + # what leave self in a state that __init__ could not have produced. + # This is a shallow copy of the instance __dict__, i.e., it stores + # references to attribute values, it does not copy the values themselves. prev_state = self.__dict__.copy() unmatched_keys = [] nested_params = defaultdict(dict) # grouped by prefix - try: - for full_key, value in params.items(): - # split full_key by first occurrence of __, if contains __ - # "key_without_dblunderscore" -> "key_without_dbl_underscore", None, None - # "key__with__dblunderscore" -> "key", "__", "with__dblunderscore" - key, delim, sub_key = full_key.partition("__") - # if key not recognized, remember for suffix matching - if key not in valid_params: - unmatched_keys += [key] - # if full_key contained __, collect suffix for component set_params - elif delim: - nested_params[key][sub_key] = value - # if key is found and did not contain __, set self.key to the value - else: - setattr(self, key, value) - valid_params[key] = value + for full_key, value in params.items(): + # split full_key by first occurrence of __, if contains __ + # "key_without_dblunderscore" -> "key_without_dbl_underscore", None, None + # "key__with__dblunderscore" -> "key", "__", "with__dblunderscore" + key, delim, sub_key = full_key.partition("__") + # if key not recognized, remember for suffix matching + if key not in valid_params: + unmatched_keys += [key] + # if full_key contained __, collect suffix for component set_params + elif delim: + nested_params[key][sub_key] = value + # if key is found and did not contain __, set self.key to the value + else: + setattr(self, key, value) + valid_params[key] = value - # all matched params have now been set - # reset object to clean post-init state with those params + # all matched params have now been set + # reset object to clean post-init state with those params + try: self.reset() - except Exception: + except Exception as e: + # restore the pre-call state, then report what happened, + # the original exception is chained via "from e" self.__dict__.clear() self.__dict__.update(prev_state) - raise + raise RuntimeError( + f"Error in {type(self).__name__}.set_params, the parameter values " + f"passed were rejected when re-running __init__, which raised " + f"{type(e).__name__}: {e}. The object has been restored to its " + f"state before the set_params call." + ) from e # recurse in components for key, sub_params in nested_params.items(): diff --git a/skbase/tests/test_base.py b/skbase/tests/test_base.py index d4cbdd58..4483d19b 100644 --- a/skbase/tests/test_base.py +++ b/skbase/tests/test_base.py @@ -930,9 +930,15 @@ def test_set_params_rolls_back_state_on_invalid_value(): """ obj = ValidatingObject(x=5) - with pytest.raises(ValueError, match="x must be non-negative"): + with pytest.raises(RuntimeError, match="restored to its state") as exc_info: obj.set_params(x=-1) + # the exception names the failing object and reports the restore + assert "ValidatingObject.set_params" in str(exc_info.value) + # the original __init__ exception is chained, not discarded + assert isinstance(exc_info.value.__cause__, ValueError) + assert "x must be non-negative" in str(exc_info.value.__cause__) + assert obj.x == 5 assert obj.get_params() == {"x": 5}