[OfficialAbhinavSingh]

Reference Issues/PRs

Fixes sktime/sktime#10695. See also sktime/sktime#10821, an earlier attempt at this issue that patched set_params on the sktime side and was closed unmerged, since the bug is in BaseObject.set_params here in skbase, not in sktime.

What does this implement/fix? Explain your changes.

BaseObject.set_params writes new parameter values to self via setattr before calling self.reset(), which is what actually 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 any later set_params call then operate on that invalid state.

class Validated(BaseObject):
    def __init__(self, x=1):
        self.x = x
        if x < 0:
            raise ValueError("x must be >= 0")
        super().__init__()

v = Validated(x=5)
v.set_params(x=-1)   # raises ValueError
v.x                   # -1, before this fix -- the rejected value stuck

Fix snapshots self.__dict__ before the parameter-writing loop in set_params, and restores it if anything in that loop or the following self.reset() call raises, then re-raises the original exception. Confirmed against the estimator that surfaced this in sktime (DilationMappingTransformer): set_params(dilation=0) now leaves dilation at its prior valid value instead of 0, and clone() succeeds afterward.

Does your contribution introduce a new dependency? If yes, which one?

No.

What should a reviewer concentrate their feedback on?

• Whether snapshotting/restoring self.__dict__ is the right mechanism, versus validating before writing to self in the first place -- the issue write-up considered both directions and this seemed the smaller, more general change, since it doesn't require touching every __init__.
• The existing test_get_params_after_set_params test in test_base.py already asserts rollback semantics under fuzz values, but the fixture class it uses (Parent) never raises in __init__, so that assertion was never actually exercised. Added a new test with a fixture that does validate, test_set_params_rolls_back_state_on_invalid_value.

Any other comments?

An LLM was used as a search and navigation tool to help trace the bug to its root cause in set_params/reset() and to draft this description. Every claim above -- the repro, the root cause, the before/after behavior, and the test counts -- was produced by running the code locally on both sides of the fix, including a run of the new test against the pre-fix source to confirm it fails without the change.

PR checklist

For all contributions

• I've reviewed the project documentation on contributing
• I've added myself to the list of contributors.
• The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
  the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.

For code contributions

• Unit tests have been added covering code functionality
• Appropriate docstrings have been added (see documentation standards)
• New public functionality has been added to the API Reference

[fkiraly]

Thanks for the contribution. I think this is potentially something to add, but the current implementation has major issues.

• storing the current state can multiply memory requirements. Imagine expensive model weights, which have to exist in two copies.
• I think "silent failure" is the wrong handling of this case. I think there should be an explicit exception at the end.

Further:

• I to not think the entire code needs to be in the try/except loop - only reset.
• a clear exception message is missing, the user will not know what has happened

[OfficialAbhinavSingh]

Thanks for the review, taking the points in order.

Only reset in the try/except — agreed. The snapshot stays above the setattr loop, since those writes are what leave the object in the bad state, but the try now wraps reset alone. Side benefit: this drops the re-indentation of the loop, so the change in _base.py is +21/-1, with self.reset() the only existing line touched.

Memory — the snapshot is self.__dict__.copy(), a shallow dict copy. It stores references to the attribute values, it does not copy the values, so model weights are not duplicated. On an object holding a 128 MB array attribute, peak allocation for an entire failed set_params call is 7632 bytes, and obj.weights is w is True after the rollback.

"Silent failure" / missing message — the previous version did re-raise (bare raise), so the exception was not swallowed, but you are right that nothing told the user a rollback had happened. It now raises:

Error in Est.set_params, the parameter values passed were rejected when
re-running __init__, which raised ValueError: alpha must be positive.
The object has been restored to its state before the set_params call.

chained from the original, so the __init__ traceback is preserved.

One point for your call: test_get_params_after_set_params catches (TypeError, ValueError), as does sklearn's check_set_params. A RuntimeError escapes both. I can keep the original exception type instead, if you would rather those checks keep seeing what they see today.

Rationale for rollback vs message-only is in sktime#10695.

[OfficialAbhinavSingh]

Went with keeping the original exception type, in a1200bb, so raise type(e)(...) rather than RuntimeError. The message and the from e chaining are unchanged, and check_set_params and test_get_params_after_set_params keep catching it.

Worth noting the skbase suite did not catch this either way, since the fixture classes do not validate in __init__, so the except branch never fires. It shows up on an object that does validate: with RuntimeError the error escapes a (TypeError, ValueError) catch, with the original type it does not.

1618 passed locally.

[OfficialAbhinavSingh]

closing this, after the discussion in sktime#10695 I don't think the rollback is worth the extra complexity in set_params. thanks for the review.