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.

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

[FIX] Fix compatibility with scikit-learn 0.23 #4768

Merged
merged 2 commits into from
May 22, 2020

Conversation

PrimozGodec
Copy link
Contributor

@PrimozGodec PrimozGodec commented May 13, 2020

Issue

The new scikit-learn release yesterday caused orange learners to start failing:

Description of changes
  • Use inspect.signature instead of __code__ to get parameters.
  • I suggest temporary pin scikit-learn to != 0.23.0.
Discussion

Should we release orange with these fixes after it is merged? Since I think we will not make a major release soon, people installing Orange with pip/conda will still get the newest version of scikit-learn. The same is for tests on add-ons. We just need a release for pip and condo. The installed do not need to be updated since it has packages packed together with orange.

Includes
  • Code changes
  • Tests
  • Documentation

@PrimozGodec PrimozGodec mentioned this pull request May 13, 2020
2 tasks
@PrimozGodec PrimozGodec changed the title [FIX] Fix bug after scikit-release [Wip][FIX] Fix bug after scikit-release May 13, 2020
@PrimozGodec PrimozGodec changed the title [Wip][FIX] Fix bug after scikit-release [Wip][FIX] Fixes after scikit release May 13, 2020
@PrimozGodec PrimozGodec changed the title [Wip][FIX] Fixes after scikit release [FIX] Fixes after scikit release May 13, 2020
@PrimozGodec PrimozGodec changed the title [FIX] Fixes after scikit release [WIP][FIX] Fixes after scikit release May 13, 2020
@PrimozGodec PrimozGodec force-pushed the fix-scikit branch 5 times, most recently from 1862d79 to 9eaf314 Compare May 13, 2020 17:44
@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #4768 into master will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4768      +/-   ##
==========================================
- Coverage   83.86%   83.81%   -0.06%     
==========================================
  Files         281      276       -5     
  Lines       56804    55900     -904     
==========================================
- Hits        47638    46850     -788     
+ Misses       9166     9050     -116     

@PrimozGodec PrimozGodec changed the title [WIP][FIX] Fixes after scikit release [FIX] Fixes after scikit release May 13, 2020
@janezd
Copy link
Contributor

janezd commented May 13, 2020

Doesn't pinning to 0.22 already do the job of fixing the test? Would it make sense to have a (temporary) pinning in a separate PR, so tests we'll pass, and then you can take your time fixing the pickling issue?

@PrimozGodec
Copy link
Contributor Author

PrimozGodec commented May 14, 2020

Doesn't pinning to 0.22 already do the job of fixing the test?

Pinning to >=0.22 does not help since the problematic version is 0.23

Would it make sense to have a (temporary) pinning in a separate PR, so tests we'll pass, and then you can take your time fixing the pickling issue?

The main reason why I pinned is k-means issue scikit-learn/scikit-learn#17208 K-means is extremely slow with the latest release. I think it should be pinned until they fix the issue and make a new release. If that will be soon maybe it is not required to merge pinning, but meanwhile, tests on PRs will fail.

For pickling, it is funny since I cannot reproduce the issue at my computer anymore. I will try on some other machine.

@PrimozGodec PrimozGodec added the needs discussion Core developers need to discuss the issue label May 19, 2020
@markotoplak
Copy link
Member

@PrimozGodec, so you suggest releasing 3.25.1 and building pypi/conda packages, which would be 3.25 with frozen scikit-learn dependency?

On conda forge we could fix requirements without doing a new release (there we can release new builds), but for pip we would need a new release.

@PrimozGodec
Copy link
Contributor Author

Yep, I think we need to fix versions on both conda and pip (for people who directly download from pip and for CI on addons). I agree with you that on conda we can easily make a new build without rising the version. On pip we can make either 3.25.1 or 3.25.0.post1.

@markotoplak markotoplak changed the title [FIX] Fixes after scikit release [FIX] Fix compatibility with scikit-learn 0.23 May 22, 2020
@markotoplak markotoplak merged commit c9cc090 into biolab:master May 22, 2020
@markotoplak markotoplak removed the needs discussion Core developers need to discuss the issue label May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants