Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt to scikit-learn 1.6 estimator tag changes #11021

Merged
merged 15 commits into from
Dec 4, 2024
+181 −26
Merged

Adapt to scikit-learn 1.6 estimator tag changes #11021

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
79ed32c
add methods in XGBModel
jameslamb Nov 24, 2024
3106cf1
implement __sklearn_tags__()
jameslamb Nov 24, 2024
3af44be
regressor tests passing
jameslamb Nov 24, 2024
a9e30b4
Merge branch 'master' of github.com:dmlc/xgboost into python/support-…
jameslamb Nov 26, 2024
6a12576
centrallize more in _more_tags(), ignore testing outputs in .gitignore
jameslamb Nov 26, 2024
52e6d83
make get_params() check less fragile
jameslamb Nov 26, 2024
816667a
formatting
jameslamb Nov 26, 2024
abfc6a6
ignore pylint warning
jameslamb Nov 26, 2024
ef725c1
fix imports in environment without scikit-learn, fix tests
jameslamb Nov 26, 2024
d845922
address pylint warnings
jameslamb Nov 26, 2024
b7564a1
remove 'pylint: disable' comments for too-many-ancestors and too-few-…
jameslamb Nov 26, 2024
8364e92
Merge branch 'master' into python/support-scikit-learn-1.6
jameslamb Dec 1, 2024
a511848
rely on mixins to set estimator_type, removed unused imports
jameslamb Dec 1, 2024
27a1bcc
Merge branch 'master' into python/support-scikit-learn-1.6
jameslamb Dec 3, 2024
18c602f
check for get_params() instead
jameslamb Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
address pylint warnings
  • Loading branch information
@jameslamb
jameslamb committed Nov 26, 2024
commit d845922acd11ea82de938a0cce395f23a10dfeb9
2 changes: 2 additions & 0 deletions python-package/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ disable = [
"import-error",
"attribute-defined-outside-init",
"import-outside-toplevel",
"too-few-public-methods",
"too-many-ancestors",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching the placeholder classes in the scikit-learn-is-not-available branch of compat.py to be individual, differently-named, empty classes (instead of all using object), led to these several of these pylint errors, in sklearn.py and dask.py:

R0901: Too many ancestors (9/7) (too-many-ancestors)
R0903: Too few public methods (0/2) (too-few-public-methods)

(build link)

It seems that there are already many other places in this codebase where those warnings are being suppressed with # pylint: disable comments. So instead of adding more such comments (some of which would have to share a line with # type: ignore comments for mypy), I'm proposing:

  • just globally ignore these pylint warnings for the whole project
  • remove any existing # pylint: disable comments about them

I don't feel that strongly... if you'd prefer to keep suppressing individual cases of these, please let me know and I'll happily switch back to #pylint: disable comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@RAMitchell find the pylint checks helpful. I myself prefer mypy checks and think the pylint is not particularly suitable for ML libraries like XGBoost. In general, I don't have a strong opinion about these "structural" or naming warnings and care mostly about warnings like unused imports or use before definition.

"too-many-nested-blocks",
"unsubscriptable-object",
"useless-object-inheritance"
Expand Down
8 changes: 0 additions & 8 deletions python-package/xgboost/compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,23 +76,15 @@ def lazy_isinstance(instance: Any, module: str, name: str) -> bool:
class XGBModelBase: # type: ignore[no-redef]
"""Dummy class for sklearn.base.BaseEstimator."""

pass

class XGBClassifierBase: # type: ignore[no-redef]
"""Dummy class for sklearn.base.ClassifierMixin."""

pass

class XGBRegressorBase: # type: ignore[no-redef]
"""Dummy class for sklearn.base.RegressorMixin."""

pass

class LabelEncoder: # type: ignore[no-redef]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the label encoder for now. It's not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh great! I just removed that in a511848

Noticed that KFold was also unused, so I removed that as well.

"""Dummy class for sklearn.preprocessing.LabelEncoder."""

pass

XGBKFold = None
XGBStratifiedKFold = None

Expand Down
Loading