-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
MAINT unpin cython in CI config #27627
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
Conversation
@@ -8,7 +8,7 @@ dependencies: | |||
- numpy=1.17.3 # min | |||
- blas[build=openblas] | |||
- scipy=1.5.0 # min | |||
- cython<3.0.0 | |||
- cython=0.29.33 # min |
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.
Since other dependencies used min dependencies on this build I decided to follow this rule for cython as well.
The doc lock file generation works fine on my machine and takes around 30 seconds. |
@lesteve can you please push the results to this PR? |
Once the CI has completed. |
I am unable to reproduce the weird scipy-dev error locally. On the other hand I get plenty of errors locally because of deprecated things in Python 3.12 some of them are not inside scikit-learn e.g. joblib, dateutil (dateutil/dateutil#1284). Probably the easiest thing to do would be to stay on Python 3.11 for scipy-dev for now (there is a chance that it fixes the scipy-dev CI if I have to guess). scipy-dev error
|
Let me push this. |
I pushed too early without waiting for the end of the PyPy tests of the previous build but they are still green after 1h55 of running, which is a net improvement compared to the last scheduled job that was crashing from the start: Still they are too slow to run to wait for the review of this PR. Let's see the results of the next nightly PyPy build after the merge of this PR. |
Yeah! the |
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.
LGTM! Thank you for the recent work on investigating performance regression with Cython 3!
I think we can create "good first Cython issue" to replace interface inclusion which has been introduced in Cython 3 such as this one:
scikit-learn/sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors.pyx.tp
Lines 22 to 26 in fb6b9f5
# TODO: change for `libcpp.algorithm.move` once Cython 3 is used | |
# Introduction in Cython: | |
# https://github.com/cython/cython/blob/05059e2a9b89bf6738a7750b905057e5b1e3fe2e/Cython/Includes/libcpp/algorithm.pxd#L47 #noqa | |
cdef extern from "<algorithm>" namespace "std" nogil: | |
OutputIt move[InputIt, OutputIt](InputIt first, InputIt last, OutputIt d_first) except + #noqa |
What do you think?
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
#27086 was closed as Cython 3.0.3 fixed all the Cython 3 related performance regressions we observed in the benchmarks.
So let's unpin Cython.
@lesteve I also updated the script to make it easy to display the commands and their outputs.
Note: the conda-lock command for
"doc"
runs indefinitely on my host, so I had to interrupt it with ctrl-C. There is no error message. I don't know how to debug this.@lesteve or someone else could you please try to see if you can reproduce?