Skip to content

Restore dmatrix.__init__ even if validate_data_initialization raises - #12398

Open
Nityahapani wants to merge 1 commit into
dmlc:masterfrom
Nityahapani:fix/validate-data-init-restore-on-exception
Open

Restore dmatrix.__init__ even if validate_data_initialization raises#12398
Nityahapani wants to merge 1 commit into
dmlc:masterfrom
Nityahapani:fix/validate-data-init-restore-on-exception

Conversation

@Nityahapani

Copy link
Copy Markdown
Contributor

Restore dmatrix.init even if validate_data_initialization raises

Summary

validate_data_initialization() monkey-patches dmatrix.__init__ (the
real, shared class passed in — e.g. the actual xgboost.QuantileDMatrix,
not a copy) with a counting wrapper, then restores the original
implementation at the very end of the function.

There was no try/finally protecting that restoration. This means: if
either of the internal fit() calls or, notably, either of the two
assert count[0] == ... checks raises — which is exactly the failure mode
this function exists to detect (a regression causing an extra DMatrix to
be constructed) — the restoration line is skipped, and the real, shared
dmatrix class stays permanently monkey-patched with the counting
wrapper for the rest of the test session. Every subsequent test constructing
that class would silently carry this instrumentation.

Why this matters

This function is called with xgb.QuantileDMatrix (the real, importable
class) in test_with_sklearn.py. If it ever catches the exact regression
it's designed to catch, the class corruption it leaves behind could make
unrelated, later test failures confusing to debug.

Change

     dmatrix.__init__ = new_init
-    model(n_estimators=1).fit(X, y, eval_set=[(X, y)])
-
-    assert count[0] == 1
-    count[0] = 0  # only 1 DMatrix is created.
-
-    y_copy = y.copy()
-    model(n_estimators=1).fit(X, y, eval_set=[(X, y_copy)])
-    assert count[0] == 2  # a different Python object is considered different
-
-    dmatrix.__init__ = old_init
+    try:
+        model(n_estimators=1).fit(X, y, eval_set=[(X, y)])
+
+        assert count[0] == 1
+        count[0] = 0  # only 1 DMatrix is created.
+
+        y_copy = y.copy()
+        model(n_estimators=1).fit(X, y, eval_set=[(X, y_copy)])
+        assert count[0] == 2  # a different Python object is considered different
+    finally:
+        dmatrix.__init__ = old_init

Wraps the body in try/finally so dmatrix.__init__ is always restored,
whether the function succeeds or one of the internal calls/assertions
raises. Also removed the now-redundant duplicate restoration line that was
left after the original final statement.

Verification

  • Before the fix: an assertion failure genuinely leaves __init__
    permanently patched (confirmed via an is not identity check against the
    original method)
  • After the fix: __init__ is correctly restored in both the exception
    case and the normal success case

mypy and ruff both pass on the modified file.

validate_data_initialization() monkey-patches dmatrix.__init__ (the
REAL, shared class passed in -- e.g. the actual xgboost.QuantileDMatrix,
not a copy) with a counting wrapper, then restores the original
implementation at the very end of the function.

There was no try/finally protecting that restoration. This means: if
either of the internal `fit()` calls or, notably, either of the two
`assert count[0] == ...` checks raises -- which is exactly the failure
mode this function exists to detect (a regression causing an extra
DMatrix to be constructed) -- the restoration line is skipped, and
the real, shared dmatrix class stays permanently monkey-patched with
the counting wrapper for the rest of the test session. Every
subsequent test constructing that class would silently carry this
instrumentation.

Fix: wrap the body in try/finally so dmatrix.__init__ is always
restored, whether the function succeeds or one of the internal calls/
assertions raises. Also removed the now-redundant duplicate
restoration line that was left after the original final statement.

Verified standalone with a toy stand-in class and both an
assertion-failure path and the normal success path:
  - before the fix: an assertion failure genuinely leaves __init__
    permanently patched (confirmed via `is not` identity check)
  - after the fix: __init__ is correctly restored in both the
    exception case and the normal success case

mypy and ruff both pass on the modified file.
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.

1 participant