sklearn compat: centralize version flags, add wrapper helpers + contributor docs, silence n_alphas leak on 1.7+ by kbattocchi · Pull Request #1047 · py-why/EconML

kbattocchi · 2026-07-16T17:06:09Z

Motivated by the review of PR #1031, this PR consolidates how EconML handles scikit-learn version differences and lays the groundwork for handling future ones consistently. Four commits, roughly in order of scope:

1. `Centralize sklearn cross-version compatibility shims` (`114932d5`)

New module econml/_sklearn_compat.py — the single home for sklearn version flags and any compatibility helpers.

SKLEARN_GE_12 / 14 / 15 / 16 / 17 / 18 flags computed once at import.
Symbol relocations that used to live inline in econml/utilities.py, econml/_ensemble/_ensemble.py, and econml/solutions/causal_analysis/_causal_analysis.py (_print_elapsed_time, _get_column_indices) now import from a single place.
Two thin function wrappers: one_hot_encoder() (papers over the sparse → sparse_output rename in 1.2) and ensure_finite_kwargs() (papers over force_all_finite= → ensure_all_finite= in 1.6, removed in 1.8).
Migrates 5 existing call sites in sklearn_extensions/linear_model.py, sklearn_extensions/model_selection.py, and _ensemble/_ensemble.py from ad-hoc parse(sklearn.__version__) blocks to the central flags.

2. `Add sklearn-compat test helpers for wrapper invariants` (`0a7b8a14`)

New module econml/tests/_sklearn_compat_helpers.py with two helpers, each designed to be a one-liner in wrapper tests:

assert_sklearn_roundtrip(cls, **kwargs) constructs an estimator with the given user-facing kwargs, verifies get_params() reports those kwargs exactly, verifies clone() produces an equivalent instance, and verifies idempotence under re-clone. Catches the specific bug class where a wrapper drops an arg from super().__init__() and the parent silently writes a "deprecated" sentinel onto self.
no_sklearn_future_warnings() context manager promotes sklearn-originated FutureWarning / DeprecationWarning to errors, so a wrapper's happy-path fit/predict tests fail loudly when an upstream deprecation starts firing.

Comes with 17 meta-tests that drive the helpers with drifted-wrapper fixtures and confirm they detect the bugs they claim to detect. Also strengthens TestLassoExtensions.test_can_clone in test_linear_model.py to use the roundtrip helper on our own weighted-lasso wrappers.

3. `Document sklearn cross-version compat conventions` (`fb81cf39`)

Adds the canonical contributor recipe in two places:

econml/_sklearn_compat.py module docstring — a 5-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly:
1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites — NOT every constructor arg, and in particular NOT an arg the parent stored under a translated name. (This is the bug Fix scikit-learn 1.7+ FutureWarnings in WeightedLassoCV and WeightedMultiTaskLassoCV #1031 introduced and fix: complete sklearn 1.9 support — drop self.alphas overwrite (closes #1032) #1042 fixed.)
2. If the parent removed the arg entirely, add a get_params override so it doesn't leak into sklearn's internal path_params / lasso_path calls. (Pattern originally suggested in FIX: resolve sklearn 1.9 compatibility for WeightedLassoCV n_alphas deprecation (#1032 ) #1046.)
Plus a short "Forward-compatibility practices" section: treat every new sklearn FutureWarning as a removal timer (the removal version is named in the message) and file it as an issue against that version rather than silencing.
README.md, new "Working with scikit-learn version differences" subsection under "For Developers" — a philosophy paragraph plus a table surveying the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split into a dedicated CONTRIBUTING.md in a future change.

4. `Centralize sklearn 1.7 dispatch in Weighted*LassoCV and silence n_alphas FutureWarnings` (`c7ddb881`)

Layered on top of the just-merged #1042. Two follow-on changes:

Replace the inline parse(sklearn.__version__) >= parse('1.7') version dispatch in WeightedLassoCV.__init__ and WeightedMultiTaskLassoCV.__init__ with the SKLEARN_GE_17 flag from _sklearn_compat.py, and factor the shared kwargs into a common_kwargs dict. Removes ~20 lines of duplicated super().__init__(...) body per class.
Add a get_params() override on both wrappers that drops n_alphas on sklearn >= 1.7. Without this override, self.get_params() still returns n_alphas because sklearn's _get_param_names inspects our wrapper's __init__ signature (which still lists n_alphas for backwards compat), not the parent's. That leaks into lasso_path(**path_params) inside sklearn's LinearModelCV.fit and triggers a per-CV-fold FutureWarning on sklearn 1.7-1.10; on sklearn 1.11 n_alphas will be removed from lasso_path and it will become a hard TypeError. Silencing here is both cosmetic (no warning storm) and forward-looking (protects us from the eventual removal). Idea originally suggested in FIX: resolve sklearn 1.9 compatibility for WeightedLassoCV n_alphas deprecation (#1032 ) #1046; credit to @genrichez for identifying it.

Also adds test_no_sklearn_future_warnings_on_happy_path_fit to TestLassoExtensions, using the no_sklearn_future_warnings() helper. Passes on the current pinned sklearn (1.4.2) and on a scratch venv with scikit-learn==1.9.0.

Verification

Local unittest run on sklearn 1.4.2 (current pinned): all targeted tests in test_linear_model.py and test_sklearn_compat_helpers.py pass (20 tests).
Manual smoke test on sklearn 1.9.0 in a scratch venv:
- import econml.orf succeeds (jakevdp's exact repro from Scikit-learn v1.9 drops n_alphas from LassoCV and related estimators, causing econml to error on import #1032)
- WeightedLassoCV(cv=3).fit(X, y) emits 0 FutureWarnings (compare: 3 warnings per fit before the get_params override)
- WeightedMultiTaskLassoCV(cv=3).fit(X, y_2d) emits 0 FutureWarnings
- clone(WeightedLassoCV(cv=3, n_alphas=5)) produces a functionally equivalent estimator (clone.alphas=5)

Closes / addresses lessons from: Scikit-learn v1.9 drops n_alphas from LassoCV and related estimators, causing econml to error on import #1032 (jakevdp's bug report), Fix scikit-learn 1.7+ FutureWarnings in WeightedLassoCV and WeightedMultiTaskLassoCV #1031 (the fix that introduced the sentinel-overwrite bug), fix: complete sklearn 1.9 support — drop self.alphas overwrite (closes #1032) #1042 (upstream fix — merged as prereq to this PR), FIX: resolve sklearn 1.9 compatibility for WeightedLassoCV n_alphas deprecation (#1032 ) #1046 (get_params override pattern — closed as superseded, but its idea is credited and adopted here).

Copilot

Pull request overview

This PR centralizes scikit-learn cross-version compatibility handling into a dedicated module, updates call sites to use shared version flags/shims, and adds test helpers + documentation to make wrapper regressions (clone/get_params drift and sklearn deprecation warning storms) easier to prevent.

Changes:

Add econml/_sklearn_compat.py as the single home for sklearn version flags and small shims (moved-symbol reexports, kwarg-rename helpers).
Refactor multiple modules (utilities, model_selection, ensemble, causal_analysis, linear_model) to consume centralized flags/helpers instead of repeating version parsing.
Add wrapper-invariant test helpers + meta-tests, and strengthen existing linear-model tests to catch wrapper drift and sklearn-originated FutureWarnings.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
README.md	Adds contributor-facing guidance on handling sklearn version differences and points to the new compat/test helper modules.
econml/utilities.py	Replaces inline sklearn-version branching for finite-check kwargs and OneHotEncoder construction with centralized compat helpers.
econml/tests/test_sklearn_compat_helpers.py	Adds meta-tests validating the new sklearn-compat test helpers behave as intended.
econml/tests/test_linear_model.py	Strengthens clone/get_params invariants and adds a “no sklearn FutureWarnings on happy-path fit” regression test.
econml/tests/_sklearn_compat_helpers.py	Introduces reusable test helpers for wrapper round-trip invariants and for promoting sklearn deprecation warnings to errors.
econml/solutions/causal_analysis/_causal_analysis.py	Switches `_get_column_indices` import to the centralized compat re-export.
econml/sklearn_extensions/model_selection.py	Replaces ad-hoc sklearn version parsing with centralized version flags for internal API branching.
econml/sklearn_extensions/linear_model.py	Centralizes sklearn>=1.7 dispatch for Weighted*LassoCV and adds get_params filtering to prevent `n_alphas` leakage.
econml/_sklearn_compat.py	New centralized module for sklearn version flags, moved-symbol re-exports, and kwarg-rename helpers, plus a contributor “recipe”.
econml/_ensemble/_ensemble.py	Switches `_print_elapsed_time` import to the centralized compat re-export.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

kbattocchi · 2026-07-17T19:03:26Z

+    def set_params(self, **params):
+        # Because ``get_params`` drops ``n_alphas`` on sklearn >= 1.7, sklearn's
+        # default ``set_params`` (which validates unknown keys against
+        # ``get_params``) would reject ``set_params(n_alphas=...)`` and any
+        # ``GridSearchCV`` / ``Pipeline`` parameter grid that names it, even
+        # though our ``__init__`` still accepts it. Preserve the legacy contract
+        # by mapping ``n_alphas`` to the equivalent ``alphas=<int>`` on
+        # sklearn >= 1.7. When both are passed, ``alphas`` takes precedence
+        # (matching the ``__init__`` dispatch semantics). Preserve
+        # ``self.n_alphas`` as the compatibility sentinel on all newer
+        # sklearn versions.
+        if SKLEARN_GE_17 and 'n_alphas' in params:
+            n_alphas_value = params.pop('n_alphas')
+            if n_alphas_value != 100:
+                _warn_n_alphas_deprecated(type(self).__name__)
+            if params.get('alphas') is None:
+                params['alphas'] = n_alphas_value
+        return super().set_params(**params)


I investigated this and am intentionally keeping the current “latest explicit setter wins” behavior.

On sklearn >=1.7, even a default wrapper already has self.alphas=100 because construction translates the legacy default. If set_params(n_alphas=5) only mapped when the current self.alphas were None, it would be a no-op on every default estimator and GridSearchCV(..., {'n_alphas': [5, 10]}) would not actually vary the grid size—the compatibility path this override exists to preserve.

The implemented semantics are:

If only n_alphas is supplied in a call, it updates alphas (latest explicit setter wins).

If both are supplied in the same call and alphas is non-None, alphas wins (matching constructor precedence).

If both are supplied and alphas=None, n_alphas supplies the effective value.

I added regression checks covering all three cases on sklearn 1.8 and 1.9 and expanded the implementation comment to make the intent explicit.

kbattocchi · 2026-07-17T19:03:16Z

+    def set_params(self, **params):
+        # See the corresponding comment on ``WeightedLassoCV.set_params``.
+        if SKLEARN_GE_17 and 'n_alphas' in params:
+            n_alphas_value = params.pop('n_alphas')
+            if n_alphas_value != 100:
+                _warn_n_alphas_deprecated(type(self).__name__)
+            if params.get('alphas') is None:
+                params['alphas'] = n_alphas_value
+        return super().set_params(**params)


Same intentional semantics as WeightedLassoCV; see the detailed reply on that thread. The shared regression test covers both wrapper classes across sklearn 1.8 and 1.9.

kbattocchi · 2026-07-17T19:03:19Z

+    if not hasattr(estimator, "get_params"):
+        raise TypeError(
+            f"{type(estimator).__name__} has no get_params() method; it is not "
+            f"sklearn-compatible."
+        )
+    original_params = estimator.get_params(deep=False)


I considered the fallback but am intentionally retaining the standard get_params(deep=False) requirement. The helper promises compatibility with sklearn.base.clone, and clone itself calls get_params(deep=False); an object whose method does not accept deep would fail later during cloning anyway. Falling back here would make the helper appear more permissive than the operation it validates.

I clarified the docstring to state explicitly that clone compatibility includes accepting the standard deep=False call.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Introduce econml/_sklearn_compat.py as the single home for sklearn version gates and compatibility helpers. Replace the scattered `parse(sklearn.__version__) >= parse(...)` blocks (previously in utilities.py, sklearn_extensions/linear_model.py, sklearn_extensions/model_selection.py, _ensemble/_ensemble.py, and solutions/causal_analysis/_causal_analysis.py) with imports of named SKLEARN_GE_* constants and helper functions from the new module. Migrated: * one_hot_encoder (moved from utilities.py, re-exported there for back-compat) * _get_ensure_finite_arg -> ensure_finite_kwargs (moved from utilities.py) * _get_column_indices and _print_elapsed_time symbol-relocation imports * SKLEARN_GE_18 used in linear_model.py and model_selection.py * SKLEARN_GE_14 used in model_selection.py No behavior change. Each version gate now appears exactly once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Keith Battocchi <kebatt@microsoft.com>

Adds econml/tests/_sklearn_compat_helpers.py with two helpers aimed at the wrapper-around-sklearn-estimator pattern used throughout econml.sklearn_extensions: * assert_sklearn_roundtrip(cls, **kwargs): construct the estimator, then assert that get_params() reports exactly the kwargs the user passed, and that clone() preserves all params. The kwargs-form check is what catches the PR-#1031 class of bug, where a wrapper omits an arg from its super().__init__() call and the parent silently writes a 'deprecated' sentinel onto self. * no_sklearn_future_warnings(): context manager that promotes sklearn-originated FutureWarning/DeprecationWarning to errors, so wrapper happy-paths fail loudly when an upstream deprecation starts firing instead of contributing to a warning storm. Both helpers are deliberately one-liner-friendly and intended to be the standard vocabulary for new sklearn-extension tests. Also strengthens econml/tests/test_linear_model.py::test_can_clone (which previously just called clone() with no assertion) to use the new helper, and adds test_clone_preserves_explicit_kwargs as an explicit kwarg-preservation check across the Weighted*/DebiasedLasso wrappers. Both pass on currently-installed sklearn (1.4.2); on sklearn>=1.7 the latter is the test that would have caught PR #1031. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Keith Battocchi <kebatt@microsoft.com>

Adds the canonical contributor recipe for working with sklearn version differences in two places: * econml/_sklearn_compat.py module docstring: expanded to (a) state why EconML supports a wide sklearn range (user environments + free CI coverage from the Python matrix) and (b) include a five-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly: 1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites with a sentinel. Do NOT reassign every constructor argument unconditionally, and in particular do NOT reassign the arg that the parent stored under a translated name -- that clobbers the parent's correct value back to the caller's default. This is the bug PR #1031 introduced and PR #1042 fixed. 2. If the parent removed the arg entirely (not just deprecated it), add a get_params override that drops the arg from the returned dict on the affected sklearn versions. Without it, sklearn's internal _get_param_names still inspects our wrapper's __init__ signature, so the removed arg leaks into path_params / lasso_path and either warns or errors depending on sklearn version. This is the pattern PR #1046 suggested. Includes matching code skeletons for the wrapper and its tests, pointing at the assert_sklearn_roundtrip / no_sklearn_future_warnings helpers. Also adds a short 'Forward-compatibility practices' section: treat every new sklearn FutureWarning / DeprecationWarning as a removal timer (the removal version is named in the message), open an issue with that version stamp, and prefer eager migration over silencing. * README.md, new 'Working with scikit-learn version differences' subsection under 'For Developers': a short framing of the broad-compat philosophy plus a table that surveys the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split out to a dedicated CONTRIBUTING.md in a future change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Keith Battocchi <kebatt@microsoft.com>

@genrichez

…cy n_alphas compatibility Layered on top of #1042, which fixed sklearn 1.9's immediate InvalidParameterError. This follow-up centralizes and completes the cross-version handling for WeightedLassoCV and WeightedMultiTaskLassoCV. Replace inline sklearn version parsing with SKLEARN_GE_17 and factor the shared parent-constructor kwargs so only the alphas/n_alphas translation branches across versions. Keep the wrappers' legacy n_alphas argument usable without leaking it into sklearn internals: - Translate n_alphas=<int> to alphas=<int> on sklearn >= 1.7. - Preserve sklearn 1.7-1.8's "deprecated" sentinel in self.n_alphas so the parent's fit-time deprecation check stays quiet. - Restore that same sentinel on sklearn 1.9+, where the parent removed the attribute, because BaseEstimator.get_params() still inspects our static backwards-compatible __init__ signature and expects the attribute to exist. - Filter n_alphas from get_params() before sklearn's internal path_params can pass it to lasso_path. - Pair that filter with a set_params() override that maps the legacy name to alphas, preserving GridSearchCV and Pipeline parameter grids. - Emit one EconML FutureWarning when users explicitly select the legacy alias, nudging them toward alphas=<int> without allowing sklearn to emit one warning per CV fold. The get_params/set_params pattern was originally suggested in #1046; credit to @genrichez for identifying it. Tests cover warning-free default fits, the clone/set_params/fit path, wrapper-level deprecation warnings, and #1042's strict-sklearn regression. The implementation was verified against sklearn 1.4.2, 1.8.0, and 1.9.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Keith Battocchi <kebatt@microsoft.com>

fverac

Looks good!

kbattocchi requested a review from Copilot July 16, 2026 17:10

Copilot started reviewing on behalf of kbattocchi July 16, 2026 17:13 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread README.md Outdated

Comment thread econml/_sklearn_compat.py Outdated

Comment thread econml/tests/test_linear_model.py Outdated

Comment thread econml/sklearn_extensions/linear_model.py

Comment thread econml/sklearn_extensions/linear_model.py

kbattocchi force-pushed the sklearn-compat-module branch 2 times, most recently from d997076 to c8c69c4 Compare July 17, 2026 00:23

kbattocchi requested a review from Copilot July 17, 2026 00:53

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread econml/tests/test_linear_model.py Outdated

Comment thread econml/tests/test_linear_model.py Outdated

Comment thread econml/_sklearn_compat.py

Copilot started reviewing on behalf of kbattocchi July 17, 2026 01:54 View session

kbattocchi force-pushed the sklearn-compat-module branch from c8c69c4 to 4fd3df5 Compare July 17, 2026 17:10

kbattocchi requested a review from Copilot July 17, 2026 17:12

Copilot started reviewing on behalf of kbattocchi July 17, 2026 17:17 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread econml/tests/_sklearn_compat_helpers.py Outdated

Comment thread econml/tests/_sklearn_compat_helpers.py Outdated

kbattocchi force-pushed the sklearn-compat-module branch from 4fd3df5 to c5d612c Compare July 17, 2026 17:34

kbattocchi requested a review from Copilot July 17, 2026 17:49

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Copilot started reviewing on behalf of kbattocchi July 17, 2026 18:13 View session

kbattocchi force-pushed the sklearn-compat-module branch from c5d612c to 3048211 Compare July 17, 2026 19:03

kbattocchi requested a review from Copilot July 23, 2026 17:08

Copilot started reviewing on behalf of kbattocchi July 23, 2026 17:09 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

kbattocchi and others added 4 commits July 23, 2026 13:51

kbattocchi force-pushed the sklearn-compat-module branch from 3048211 to 6ae1dd3 Compare July 23, 2026 18:02

kbattocchi requested a review from fverac July 23, 2026 18:05

kbattocchi mentioned this pull request Jul 23, 2026

Support sklearn 1.8 #1019

Open

fverac approved these changes Jul 29, 2026

View reviewed changes

kbattocchi merged commit 927ac26 into main Jul 30, 2026
114 checks passed

kbattocchi deleted the sklearn-compat-module branch July 30, 2026 14:24

Uh oh!

Conversation

kbattocchi commented Jul 16, 2026

1. Centralize sklearn cross-version compatibility shims (114932d5)

2. Add sklearn-compat test helpers for wrapper invariants (0a7b8a14)

3. Document sklearn cross-version compat conventions (fb81cf39)

4. Centralize sklearn 1.7 dispatch in Weighted*LassoCV and silence n_alphas FutureWarnings (c7ddb881)

Verification

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

kbattocchi Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

kbattocchi Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kbattocchi Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

fverac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. `Centralize sklearn cross-version compatibility shims` (`114932d5`)

2. `Add sklearn-compat test helpers for wrapper invariants` (`0a7b8a14`)

3. `Document sklearn cross-version compat conventions` (`fb81cf39`)

4. `Centralize sklearn 1.7 dispatch in Weighted*LassoCV and silence n_alphas FutureWarnings` (`c7ddb881`)