-
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
[dask] [ci] add support for scikit-learn 0.24+ in tests (fixes #4031) #4032
Conversation
if sk_version > parse_version("0.23"): | ||
Estimator = estimator | ||
else: | ||
Estimator = estimator.__class__ | ||
sklearn_checks.check_parameters_default_constructible(name, Estimator) |
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.
I did try just passing an instance always, so we could avoid parsing the scikit-learn version, but it looks like scikit-learn
0.22.x only supported passing a class in this method 😆
Here's what I got changing the code to the following on scikit-learn==0.22.0
@pytest.mark.parametrize("estimator", list(_tested_estimators()))
def test_parameters_default_constructible(estimator):
name = estimator.__class__.__name__
sklearn_checks.check_parameters_default_constructible(name, estimator)
E TypeError: 'DaskLGBMRegressor' object is not callable
"Fast" update, conda! 👍 😄 But at https://anaconda.org/anaconda/scikit-learn I don't understand why we get |
I hope some time we just drop all workarounds and leave only unified version of tests that assumes we use |
Co-authored-by: Nikita Titov <[email protected]>
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.
Great job! Many thanks!
ugh, more MiKTeX issues. this one is new.
I can see that that isn't true, and wasn't true the last time I restarted the failing Windows R jobs about 30 minutes ago. I'm going to try upgrading to miktex-setup 4.1 (https://ctan.math.illinois.edu/systems/win32/miktex/setup/windows-x86/). It looks like a new minor version was released a few weeks ago. https://ctan.math.illinois.edu/systems/win32/miktex/setup/windows-x86/ I'll just do that on this PR, since it's blocking anyway. |
upgrading MikTeX did not work. Same error as before. https://github.com/microsoft/LightGBM/pull/4032/checks?check_run_id=2005836771
But I can clearly see that that URL is responding ok. and I get a 200 hitting it directly with |
There are not any open issues at https://github.com/MiKTeX/miktex/issues related to this. The answers on https://tex.stackexchange.com/questions/251242/unable-to-connect-to-repository-in-miktex-2-9 (from a few years ago), suggest that this error isn't about the availability of any one mirror. There is an answer there with 12 upvotes that says
In November 2019, on a related issue (MiKTeX/miktex#414 (comment)), one of the MiKTeX maintainers said the following
I can confirm that that is down
returns
This seems like an issue similar to #4005 , where all of the MiKTeX ecosystem is stuck until something central is updated :/. I don't have time right now to investigate this further, but if it isn't resolved by tonight (it's currently 11am in my timezone), I can pick up my exploration of |
One more update. I just tried pushing a change that points our CI code at a different package repository. MiKTeX/miktex#414 mentioned that you could use https://miktex.org/pkg/repositories to find the list of package repositories. I noticed that the I pushed 4845136 changing package repo URLs, just to check if that matters at all. |
Seems this is still a problem. I found another relevant issue. The exact same thing seems to have happened in 2018. Similar response from the maintainer as we've seen in other MiKTeX issues, basically "oh this is just broken for a day or two" 😫 |
@StrikerRUS thanks for rebuilding! I'm glad this started working again. Once we're done with the release (#3872 ), I'll spend some time working on some options to hopefully avoid this in the future. It's awful having the whole project's CI blocked for as long as two days just because of the tools needed for the R documentation on Windows. |
Great, thank you! |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Fixes #4031. In
scikit-learn
0.24.x, support for passing a class (instead of an instance) tosklearn.utils.estimator_checks.check_parameters_default_constructible()
was removed (scikit-learn/scikit-learn#17134).We must have recently started getting
scikit-learn
>=0.24.0 in tests, because CI has been failing on this test with this errorThis error only shows up for the Dask estimators, because we call
check_parameters_default_constructible()
directly there. The equivalent test intests/python_package_test/test_sklearn.py
isn't failing because it's skipped onsklearn>=0.24.0
(LightGBM/tests/python_package_test/test_sklearn.py
Line 1187 in 6356e65
sklearn.utils.estimator_checks.parametrize_with_checks()
:LightGBM/tests/python_package_test/test_sklearn.py
Line 1180 in 6356e65