-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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.
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
[python-package][R-package] adapt to scikit-learn 1.6 testing changes, pin more packages in R 3.6 CI jobs #6718
Conversation
) | ||
|
||
.install_packages(c( | ||
"brio/brio_1.1.4.tar.gz" # nolint: non_portable_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these hard-coded versions are the latest for which there's a package at https://cran.r-project.org/src/contrib/Archive. That provides a set of packages that were all working together as of a few days ago.
We shouldn't need to actively manage this list... this should be able to remain untouched (I hope) until we drop R 3 support and delete this script entirely.
_sklearn_ClassifierTags = None | ||
_sklearn_RegressorTags = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these defined again here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because if an earlier ImportError
happens, then this part above would never get evaluated:
except ImportError:
_sklearn_ClassifierTags = None
_sklearn_RegressorTags = None
And then the from .compat imprt _sklearn_ClassifierTags
in sklearn.py
would fail. This is not new, just following the pattern that's existed in LightGBM for a long time (see all the other {something} = None
above it).
Happy to consider something else if you have a recommendation for improving this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, sorry.
The first one is defined for scikit-learn<1.6 and the second when scikit-learn isn't installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah exactly. No need to be sorry, it's confusing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your hard exploration!
Just two minor suggestions.
Co-authored-by: Nikita Titov <[email protected]>
Fixes #6717, adapting
lightgbm
to the following changes inscikit-learn
(currently visible inscikit-learn
1.6 nightlies).from scikit-learn/scikit-learn#30122:
tags.estimator_type
in the tags returns by__sklearn_tags__()
tags.regressor_tags
with a non-None
valuestags.classifier_tags
with a non-None
valuesfrom scikit-learn/scikit-learn#30149:
@parametrize_with_checks()
ignores_xfail_checks
in_more_tags()
/__sklearn_tags__()
on estimators... projects must now pass a function that generates a dictionary of xfailed checks to@parametrize_with_checks()
in test codeFixes #6719, pinning all the versions of dependencies installed in R 3.6 CI jobs to make those jobs more stable (see previous issues: #6442 (comment), #6434).