Fix Issue 2114: Accept estimators that don't inherit from sklearn.BaseEstimator in tabular_pipeline by ljdyer · Pull Request #2225 · skrub-data/skrub

ljdyer · 2026-07-19T12:53:27Z

Fix Issue 2114: Accept estimators that don't inherit from sklearn.BaseEstimator in tabular_pipeline

Description

All suggestions in the issue comments made sense, so I implemented them as specified. There were three main changes:

Loosened the requirement for estimator to inherit from sklearn.BaseEstimator

Per the suggestion in the issue comments, the new implementation is to require a 'scikit-learn compatible' estimator, which is defined as an object that has get_params and set_params attributes
Check made explicit with is_scikit_learn_compatible variable
isinstance(estimator, type) check carried out before is_scikit_learn_compatible check since the validity check no longer makes sense for types. Class name no longer included in error message, since it may be displayed even for invalid types. Still serves its purpose of helping the user to spot the mistake.
Tested (in test_sklearn_incompatible_learner_fails and test_sklearn_incompatible_learner_succeeds) with dummy classes that define get_params/set_params.

Replaced HGBT and tree ensemble estimator checks with class name-based checks

Per suggestion in issue comments, replaced specific isinstance checks with broader substring match checks to determine need to HGBT/tree ensemble treatment (HistGradientBoosting, RandomForest)
This ensures compatibility with libraries that use same class names as scikit-learn (e.g. cuml)
XGB and LGBM included in substrings for tree ensemble check, for compatibility with xgboost and lightgbm libraries
Checks are made explicit (_is_hgbt_estimator, _is_tree_ensemble_estimator)
Existing tests pass with no modifications.
Added tests for tree ensemble treatment for dummy classes with RandomForest/XGB in their names

Added parameters to allow the caller to request tree ensemble/HBGT treatment, overriding default heuristics

Added is_hgbt_estimator/is_tree_ensemble_estimator parameters, per the suggestion in the issue comments
Explained in docstrings that default heuristics still apply and will work in most cases, but that these parameters can be used for overrides
Added test to verify that is_tree_ensemble_estimator results in tree ensemble treatment in a case where this would not be triggered by the default heuristic

Addresses #2114

Checklist

[ X ] I have read the contributing guidelines
[ X ] I have added tests that verify the bug fix
[ X ] I have added an entry to CHANGES.rst describing the fix
[ X ] My code follows the code style of this project
[ X ] I have checked my code and corrected any misspellings

How Has This Been Tested?

All existing and new tests in test_tabular_pipeline.py
(+ entire test suite)

AI Disclosure

[ X ] This PR contains AI-generated code
- [ X ] I have tested the code generated in my PR
- [ X ] I have read and understood every line that has been generated by the AI agent
- [ X ] I can explain what the AI-generated code does

jeromedockes

thank you very much @ljdyer !! this looks great.

I think we can remove the parameters that override the kind of estimator for now, because we would need to discuss more what are the classes of estimator and the parameter names
the tests can be simplified slightly

looks great otherwise, thanks :)

jeromedockes · 2026-07-23T13:53:36Z

        means 1 unless in a joblib ``parallel_backend`` context. ``-1`` means using all
        processors.

+    is_tree_ensemble_estimator : bool, default=None


sorry that wasn't clear from the issue discussion, but let's keep things simple for now and not add those extra parameters

jeromedockes · 2026-07-23T13:54:51Z

-            " or 'classifier' as its first argument."
+            "tabular_pipeline expects a scikit-learn compatible estimator as its first"
+            " argument. The estimator object must have 'get_params' and 'set_params'"
+            " attributes."


can we add 'fit' and 'predict' to the list? thanks!

No problem, will do.
Just want to point out that this makes the requirements a bit more stringent that previously with BaseEstimator inheritance. For example AgglomerativeClustering inherits from BaseEstimator and has get_params/set_params/fit but does not have have predict. But perhaps we want to exclude it or are not concerned because it is neither a regressor or a classifier?
Another example that has the first three but does not have predict is PCA, though this does not inherit from BaseEstimator.

jeromedockes · 2026-07-23T13:59:05Z

@@ -285,7 +311,7 @@ def tabular_pipeline(estimator, *, n_jobs=None):
    if not is_estimator_from_tabicl:
        if not get_tags(estimator).input_tags.allow_nan:


here we could be a bit more lenient on input estimators that don't implement the full scikit-learn api with something like

try: allow_nan = get_tags(estimator).input_tags.allow_nan except AttributeError: allow_nan = False # assume we need imputation if not allow_nan: steps.append(SimpleImputer(add_indicator=True))

Looks good - implementing more or less as you wrote

jeromedockes · 2026-07-23T13:59:51Z

 )


+def fake_get_tags(_):


with the catching tag attribute error suggested below we can remove this

jeromedockes · 2026-07-23T14:03:51Z

+        tabular_pipeline(sklearn_incompatible_learner)
+
+
+def test_sklearn_compatible_learner_succeeds(monkeypatch):


can we create a real estimator instead,

class Regressor: def fit(self, X, y=None): return self def predict(self, X): return np.zeros(X.shape[0]) def get_params(self): return {} def set_params(self, **params): return self then make a pipeline and check we can call fit and predict on toy data and get the expected prediction? thanks

I was able to add the fit test with a pandas DataFrame, but with predict we run into issues as check_is_fitted gets called. Could monkeypatch, but in the interest of test simplicity could we skip the test with toy data?

jeromedockes · 2026-07-23T14:05:30Z

+def test_tree_ensemble_treatment_for_any_random_forest(monkeypatch):
+    """Test that special treatment for tree ensemble models is applied when
+    substring 'RandomForest' appears in estimator class name"""
+    IAmARandomForestEstimator = type(


in those instead of calling type we can subclass the dummy regressor defined earlier eg

class UserRandomForest(Regressor): pass

Great advice - makes the tests much simpler

Sanjays2402 · 2026-07-23T22:58:42Z

-    if not isinstance(estimator, BaseEstimator):
+
+    is_scikit_learn_compatible = hasattr(estimator, "get_params") and hasattr(
+        estimator, "set_params"


This accepts non-callable attributes, so an object with get_params = 1 and set_params = 1 passes the guard but the returned pipeline raises TypeError as soon as get_params() or clone() touches it. Should these checks use callable(getattr(..., None)) instead?

Hi Sanjay, I agree that it's fine to check for callable( as well. Extending this to fit and predict too. Only caveat is what I wrote in my response to Jerome's comment above about estimators that don't implement predict.

ljdyer · 2026-07-27T19:35:46Z

Thanks @jeromedockes for the feedback! I'll be working on this again in the next few days. Cheers

ljdyer · 2026-07-29T08:46:17Z

Thank you @jeromedockes for the review. I have resubmitted so please review again when convenient.

jeromedockes · 2026-08-07T01:54:49Z

thanks a lot for making the changes @ljdyer ! I will look at it again in the next few days :)

ljdyer added 6 commits July 19, 2026 11:26

Loosen estimator requirement from BaseEstimator inheritance

6dc8195

Use monkeypatch for test_sklearn_compatible_learner_succeeds

55193ea

Detect HGBT and tree ensembles by estimator class names

8ee4a56

Add estimator type override parameters

1a28685

Applied minor updates to documentation for tabular_pipeline

253a989

Changelog

083cfe9

ljdyer changed the title ~~Fix issue 2114~~ Fix Issue 2114: Accept estimators that don't inherit from sklearn.BaseEstimator in tabular_pipeline Jul 19, 2026

Add PR number to changelog

f87d75a

jeromedockes reviewed Jul 23, 2026

View reviewed changes

Sanjays2402 reviewed Jul 23, 2026

View reviewed changes

ljdyer added 4 commits July 29, 2026 08:01

Merge changes from upstream.main

bed26b5

Undo estimator type override parameter addition

1dde1b3

Reinstate docstrings for test functions from previous commit

50d4a77

Apply feedback from PR skrub-data#2225 comments

46a1a76

Update CHANGES.rst

7ca069c

		@@ -285,7 +311,7 @@ def tabular_pipeline(estimator, *, n_jobs=None):
		if not is_estimator_from_tabicl:
		if not get_tags(estimator).input_tags.allow_nan:

		tabular_pipeline(sklearn_incompatible_learner)


		def test_sklearn_compatible_learner_succeeds(monkeypatch):

Uh oh!

Conversation

ljdyer commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix Issue 2114: Accept estimators that don't inherit from sklearn.BaseEstimator in tabular_pipeline

Description

Checklist

How Has This Been Tested?

AI Disclosure

Uh oh!

jeromedockes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljdyer Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljdyer commented Jul 27, 2026

Uh oh!

ljdyer commented Jul 29, 2026

Uh oh!

jeromedockes commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ljdyer commented Jul 19, 2026 •

edited

Loading

ljdyer Jul 29, 2026 •

edited

Loading