[python] Re-enable scikit-learn 0.22+ support #2949

StrikerRUS · 2020-03-27T01:33:50Z

Skip failing check_no_attributes_set_in_init test, refer to #2628 (comment).

Minor refactoring to pass new check_sample_weights_not_an_array test.

Reverted #2637. Fixed #2628. Closed #2946.

This reverts commit d100277.

.ci/test.sh

StrikerRUS · 2020-03-27T01:36:26Z

@jameslamb Can you please help with failing R test on macOS?

Installing packages into ‘/Users/travis/Rlib’
(as ‘lib’ is unspecified)
Warning: unable to access index for repository https://cran.rstudio.com/bin/macosx/el-capitan/contrib/3.6:
  cannot open URL 'https://cran.rstudio.com/bin/macosx/el-capitan/contrib/3.6/PACKAGES'
   packages ‘data.table’, ‘jsonlite’, ‘Matrix’, ‘R6’, ‘testthat’ are available as source packages but not as binaries
Warning message:
packages ‘data.table’, ‘jsonlite’, ‘Matrix’, ‘R6’, ‘testthat’ are not available (as a binary package for R version 3.6.1)

This reverts commit 8b4db08.

StrikerRUS · 2020-03-27T14:34:51Z

@jameslamb I tried to change CRAN repo
echo 'options(repos = "https://cran.rstudio.com")' > ${HOME}/.Rprofile -> echo 'options(repos = "https://cloud.r-project.org")' > ${HOME}/.Rprofile

The same error:

Installing packages into ‘/Users/travis/Rlib’
(as ‘lib’ is unspecified)
Warning: unable to access index for repository https://cloud.r-project.org/bin/macosx/el-capitan/contrib/3.6:
  cannot open URL 'https://cloud.r-project.org/bin/macosx/el-capitan/contrib/3.6/PACKAGES'
   packages ‘data.table’, ‘jsonlite’, ‘Matrix’, ‘R6’, ‘testthat’ are available as source packages but not as binaries
Warning message:
packages ‘data.table’, ‘jsonlite’, ‘Matrix’, ‘R6’, ‘testthat’ are not available (as a binary package for R version 3.6.1)

jameslamb · 2020-03-27T15:05:59Z

@jameslamb I tried to change CRAN repo
echo 'options(repos = "https://cran.rstudio.com")' > ${HOME}/.Rprofile -> echo 'options(repos = "https://cloud.r-project.org")' > ${HOME}/.Rprofile

The same error:

Installing packages into ‘/Users/travis/Rlib’
(as ‘lib’ is unspecified)
Warning: unable to access index for repository https://cloud.r-project.org/bin/macosx/el-capitan/contrib/3.6:
  cannot open URL 'https://cloud.r-project.org/bin/macosx/el-capitan/contrib/3.6/PACKAGES'
   packages ‘data.table’, ‘jsonlite’, ‘Matrix’, ‘R6’, ‘testthat’ are available as source packages but not as binaries
Warning message:
packages ‘data.table’, ‘jsonlite’, ‘Matrix’, ‘R6’, ‘testthat’ are not available (as a binary package for R version 3.6.1)

looking right now

jameslamb · 2020-03-28T02:59:26Z

If you rebase to master, to get the changes from #2954 , the R test issues should be resolved.

StrikerRUS · 2020-03-30T15:25:33Z

@henry0312 Please take a look when have time

henry0312 · 2020-03-30T15:27:04Z

Sure, I'm going to check tomorrow

henry0312 · 2020-04-01T11:38:42Z

python-package/setup.py

@@ -276,7 +276,7 @@ def run(self):
          install_requires=[
              'numpy',
              'scipy',
-              'scikit-learn<=0.21.3'
+              'scikit-learn!=0.22.0'


is this 'scikit-learn~=0.22,!=0.22.0'?

Sorry, I can't find ~ in the setuptools docs. Can you please clarify what do you mean?

A version specifier is one of the operators <, >, <=, >=, == or !=, followed by a version identifier.
https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-dependencies

Version specifiers
https://www.python.org/dev/peps/pep-0440/#version-specifiers

OK. I guess that this specifier is not compatible with setuptools as it is not listed there, is it?

Also, why do we need it? We are compatible with all (reasonable) versions except 0.22.0.

ping @henry0312

@henry0312

though I don't think the policy is common in software engineering.

It's kind of debatable statements. And I don't think that this PR is a good place to discuss it. I think we should create a separate issue where we should at least get opinions of other maintainers before changing the current policy.

we should check its compatibility and will add suuport of it in series.

In ideal situation, yes! You are absolutely right! But given that we are lacking human resources to do it in time, I suppose that it will be simply abandoned in the near future. Which will result in very old dependencies of LightGBM.

Unless you're ready to release a new version of LightGBM every time a new minor version or patch for a dependency is released, of course.

Absolutely agree! It is definitely not our case when only 2-3 maintainers work actively at the same time.

@ekerazha @StrikerRUS I don't mean all of the future versions wont' be allowed, but only the future versions that can breaks backward compatibility (in other words, a release changes major version), which version specifiers (i.e. scikit-learn~=0.22,!=0.22.0 or scikit-learn>=0.22, == 0.*) realizes.
And is LightGBM an almost end-user products? I think so.

I don't think sklearn is following SemVer.

oh, really...? Actually, I'm not sure.

@henry0312

And is LightGBM an almost end-user products? I think so.

LightGBM is a library, a travel website which uses LightGBM to recommend travels would be the end-user product.

oh, really...? Actually, I'm not sure.

scikit-learn had breaking API changes in minor versions, so it's not following SemVer.

@henry0312 I guess 0.22 is a minor release, but it broke backward compatibility.

Again, please let's move all our discussion to a separate issue where everyone will be able to comment. It's very hard to find it here and I want to merge this PR ASAP to enable the latest scikit-learn support.

henry0312 · 2020-04-08T10:33:49Z

By the way, why is there the same (similar?) PR, #2946?

StrikerRUS · 2020-04-08T13:51:43Z

@henry0312

By the way, why is there the same (similar?) PR, #2946?

That PR only reverts version constraint. This one fixes some new tests in addition. Refer to #2946 (comment).

henry0312 · 2020-04-08T14:39:54Z

@henry0312

By the way, why is there the same (similar?) PR, #2946?

That PR only reverts version constraint. This one fixes some new tests in addition. Refer to #2946 (comment).

Thank you for your explanation. I see 😄

guolinke · 2020-04-09T02:01:13Z

@StrikerRUS @henry0312 is this ready to merge?

henry0312 · 2020-04-09T03:04:10Z

@guolinke @StrikerRUS yes it is.

StrikerRUS added 4 commits March 27, 2020 03:17

Revert "specify the last supported version of scikit-learn (#2637)"

1bc58cb

This reverts commit d100277.

ban scikit-learn 0.22.0 and skip broken test

2acdba5

fix updated test

66cf2df

fix lint test

8b4db08

StrikerRUS requested review from chivee, guolinke, henry0312, huanzhang12, jameslamb, Laurae2 and wxchan as code owners March 27, 2020 01:33

StrikerRUS commented Mar 27, 2020

View reviewed changes

.ci/test.sh Outdated Show resolved Hide resolved

Revert "fix lint test"

22d4076

This reverts commit 8b4db08.

jameslamb mentioned this pull request Mar 27, 2020

[ci] changed CRAN mirror to fix failing installs #2954

Merged

StrikerRUS added the feature label Mar 27, 2020

Merge branch 'master' into sklearn_update

4aa3e64

jameslamb mentioned this pull request Mar 31, 2020

[ci] reduce duplication in Python Windows CI #2962

Merged

henry0312 reviewed Apr 1, 2020

View reviewed changes

StrikerRUS added 2 commits April 1, 2020 21:05

fixed conflicts

ce98f1d

Merge branch 'master' into sklearn_update

b57dcfe

StrikerRUS added awaiting response and removed awaiting response labels Apr 7, 2020

henry0312 approved these changes Apr 9, 2020

View reviewed changes

StrikerRUS mentioned this pull request Apr 9, 2020

[RFC] change strategy of defining LightGBM's dependencies #2987

Closed

henry0312 merged commit c633c6c into master Apr 10, 2020

henry0312 deleted the sklearn_update branch April 10, 2020 03:53

h-vetinari mentioned this pull request Apr 10, 2020

scikit-learn 0.22 incompatability conda-forge/lightgbm-feedstock#21

Closed

henry0312 mentioned this pull request May 12, 2020

v3.0.0rc1 #3071

Merged

lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[python] Re-enable scikit-learn 0.22+ support #2949

[python] Re-enable scikit-learn 0.22+ support #2949

StrikerRUS commented Mar 27, 2020

StrikerRUS commented Mar 27, 2020

StrikerRUS commented Mar 27, 2020

jameslamb commented Mar 27, 2020

jameslamb commented Mar 28, 2020

StrikerRUS commented Mar 30, 2020

henry0312 commented Mar 30, 2020

henry0312 Apr 1, 2020

StrikerRUS Apr 1, 2020

henry0312 Apr 1, 2020

StrikerRUS Apr 1, 2020

StrikerRUS Apr 3, 2020

StrikerRUS Apr 8, 2020 •

edited

Loading

StrikerRUS Apr 8, 2020

henry0312 Apr 8, 2020

ekerazha Apr 8, 2020 •

edited

Loading

StrikerRUS Apr 8, 2020

henry0312 commented Apr 8, 2020

StrikerRUS commented Apr 8, 2020

henry0312 commented Apr 8, 2020

guolinke commented Apr 9, 2020

henry0312 commented Apr 9, 2020

[python] Re-enable scikit-learn 0.22+ support #2949

[python] Re-enable scikit-learn 0.22+ support #2949

Conversation

StrikerRUS commented Mar 27, 2020

StrikerRUS commented Mar 27, 2020

StrikerRUS commented Mar 27, 2020

jameslamb commented Mar 27, 2020

jameslamb commented Mar 28, 2020

StrikerRUS commented Mar 30, 2020

henry0312 commented Mar 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrikerRUS Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekerazha Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henry0312 commented Apr 8, 2020

StrikerRUS commented Apr 8, 2020

henry0312 commented Apr 8, 2020

guolinke commented Apr 9, 2020

henry0312 commented Apr 9, 2020

StrikerRUS Apr 8, 2020 •

edited

Loading

ekerazha Apr 8, 2020 •

edited

Loading