Skip to content

Fix get_params() mutating and being non-idempotent for RNG random_state - #12367

Open
Nityahapani wants to merge 3 commits into
dmlc:masterfrom
Nityahapani:fix/get-params-random-state-mutation
Open

Fix get_params() mutating and being non-idempotent for RNG random_state#12367
Nityahapani wants to merge 3 commits into
dmlc:masterfrom
Nityahapani:fix/get-params-random-state-mutation

Conversation

@Nityahapani

@Nityahapani Nityahapani commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Fix get_params() mutating and being non-idempotent for RNG random_state

Summary

XGBModel.get_params() converts a np.random.RandomState or
np.random.Generator random_state into a plain int for the native
learner. It did this by calling .randint() / .integers() directly on
the RNG object
— which:

  1. Mutates the caller's own RNG object as a side effect of what should
    be a pure accessor call, advancing its internal state
  2. Makes get_params() non-idempotent — calling it twice on the same
    fitted estimator returns a different random_state value each time,
    since each call draws fresh randomness

Why this matters

sklearn's own get_params()/clone() contract assumes get_params() is a
pure, idempotent accessor — GridSearchCV, cross_val_score, and any code
path that clones or introspects the estimator relies on this. It also
silently advances a RandomState/Generator object that the calling code
may still be using elsewhere, which is a surprising side effect for a
getter to have.

I confirmed via sklearn.base.BaseEstimator.get_params() that the returned
dict holds the same object reference as self.random_state, not a
copy:

>>> from sklearn.base import BaseEstimator
>>> class Dummy(BaseEstimator):
...     def __init__(self, random_state=None):
...         self.random_state = random_state
>>> rs = np.random.RandomState(42)
>>> d = Dummy(random_state=rs)
>>> d.get_params()["random_state"] is rs
True

So .randint()/.integers() genuinely mutates the user's own RNG object,
not a throwaway copy.

Change

Derive a stable seed from the RNG's internal state (read-only) instead of
drawing a new random value:

  • RandomState: use get_state(legacy=True), which returns numpy's
    classic (str, ndarray, int, int, float) tuple for an MT19937 generator
    — verified this is numpy's actual runtime default via
    help(np.random.RandomState.get_state), even though the .pyi type
    stub for the no-arg call declares a dict[str, Any] return type (a
    stub/runtime mismatch). The explicit cast() documents this.
  • Generator (PCG64): read bit_generator.state["state"]["state"]
    directly — same non-consuming approach.

This is the same fix pattern used by at least one other sklearn-compatible
library ([PySR](https://huggingface.co/spaces/MilesCranmer/PySR/commit/dca10d6fc6a9f3237095bdea2a363770b1152f01))
for the identical bug — switching from a consuming .randint() call to
reading get_state()[1][0].

Verification

I reproduced the exact logic standalone and verified:

  • Idempotency: calling the logic twice on the same RandomState /
    Generator object now returns the identical value both times
  • Non-mutation: drew a value from a fresh RNG with the same seed
    before and after running the fixed logic — got identical draws,
    confirming the RNG's internal state was not advanced (for both
    RandomState and Generator)
  • Bounds: checked across a range of seeds including edge values (0,
    2**31-1, 2**32-1) to confirm results stay in the intended
    [0, int32_max) range
  • Passthrough: plain int random_state values are unaffected

mypy and ruff both pass on the modified file.

XGBModel.get_params() converts a np.random.RandomState or
np.random.Generator random_state into a plain int for the native
learner. It did this by calling .randint() / .integers() directly on
the RNG object -- which both (a) mutates the caller's own RNG object
as a side effect of what should be a pure accessor call, advancing its
internal state, and (b) makes get_params() non-idempotent: calling it
twice on the same fitted estimator returns a *different*
random_state value each time, since each call draws fresh randomness.

This matters because sklearn's own get_params()/clone() contract
assumes get_params() is a pure, idempotent accessor -- GridSearchCV,
cross_val_score, and any code path that clones or introspects the
estimator relies on this. It also silently advances a RandomState/
Generator object that the calling code may still be using elsewhere,
which is a surprising side effect for a getter to have.

Confirmed via sklearn.base.BaseEstimator.get_params(): the returned
dict holds the *same object reference* as self.random_state, not a
copy, so .randint()/.integers() genuinely mutates the user's own RNG.

Fix: derive a stable seed from the RNG's internal state (read-only)
instead of drawing a new random value:
  - RandomState: use get_state(legacy=True), which returns numpy's
    classic (str, ndarray, int, int, float) tuple for an MT19937
    generator -- verified this is numpy's actual runtime default via
    help(np.random.RandomState.get_state), even though the .pyi type
    stub for the no-arg call declares a dict[str, Any] return type
    (a stub/runtime mismatch); the explicit cast() documents this.
  - Generator (PCG64): read bit_generator.state['state']['state']
    directly, same non-consuming approach.

This is the same fix pattern used by at least one other sklearn-
compatible library (PySR) for the identical bug: 'fix: potential
issue with non-standard random states', switching from a consuming
.randint() call to reading get_state()[1][0].

Verified standalone (import xgboost requires the compiled C++ core,
which isn't buildable in this sandbox):
  - RandomState: get_params() logic now returns the identical value
    on repeated calls (idempotent)
  - RandomState: the object is provably NOT advanced -- drew a value
    from a fresh RNG with the same seed before and after running the
    logic, got identical results
  - Same two checks repeated for np.random.Generator
  - Bounds-checked across a range of seeds including edge values
    (0, 2**31-1, 2**32-1) to confirm results stay in the intended
    [0, int32_max) range
  - Plain int random_state values still pass through unchanged

mypy and ruff both pass on the modified file.
@trivialfis

Copy link
Copy Markdown
Member

Maybe it's better to use the integers function in setter instead of getters?

@Nityahapani

Copy link
Copy Markdown
Contributor Author

Fix get_params() mutating and being non-idempotent for RNG random_state

Summary

XGBModel.get_params() converts a np.random.RandomState or
np.random.Generator random_state into a plain int for the native
learner. It did this by calling .randint() / .integers() directly on
the RNG object
— which:

  1. Mutates the caller's own RNG object as a side effect of what should
    be a pure accessor call, advancing its internal state
  2. Makes get_params() non-idempotent — calling it twice on the same
    fitted estimator returns a different random_state value each time,
    since each call draws fresh randomness

Why this matters

sklearn's own get_params()/clone() contract assumes get_params() is a
pure, idempotent accessor — GridSearchCV, cross_val_score, and any code
path that clones or introspects the estimator relies on this. It also
silently advances a RandomState/Generator object that the calling code
may still be using elsewhere, which is a surprising side effect for a
getter to have.

I confirmed via sklearn.base.BaseEstimator.get_params() that the returned
dict holds the same object reference as self.random_state, not a
copy:

>>> from sklearn.base import BaseEstimator
>>> class Dummy(BaseEstimator):
...     def __init__(self, random_state=None):
...         self.random_state = random_state
>>> rs = np.random.RandomState(42)
>>> d = Dummy(random_state=rs)
>>> d.get_params()["random_state"] is rs
True

So .randint()/.integers() genuinely mutates the user's own RNG object,
not a throwaway copy.

Change

Derive a stable seed from the RNG's internal state (read-only) instead of
drawing a new random value:

  • RandomState: use get_state(legacy=True), which returns numpy's
    classic (str, ndarray, int, int, float) tuple for an MT19937 generator
    — verified this is numpy's actual runtime default via
    help(np.random.RandomState.get_state), even though the .pyi type
    stub for the no-arg call declares a dict[str, Any] return type (a
    stub/runtime mismatch). The explicit cast() documents this.
  • Generator (PCG64): read bit_generator.state["state"]["state"]
    directly — same non-consuming approach.
    This is the same fix pattern used by at least one other sklearn-compatible
    library (PySR)
    for the identical bug — switching from a consuming .randint() call to
    reading get_state()[1][0].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants