Skip to content

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

Merged
merged 8 commits into from
Oct 21, 2023
Merged

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Oct 20, 2023

#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?

@@ -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
Copy link
Member Author

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.

@github-actions
Copy link

github-actions bot commented Oct 20, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 6ba70c2. Link to the linter CI: here

@ogrisel ogrisel changed the title Ci unpin cython MAINT unpin cython in CI config Oct 20, 2023
@lesteve
Copy link
Member

lesteve commented Oct 20, 2023

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.

The doc lock file generation works fine on my machine and takes around 30 seconds.

@ogrisel
Copy link
Member Author

ogrisel commented Oct 20, 2023

@lesteve can you please push the results to this PR?

@ogrisel
Copy link
Member Author

ogrisel commented Oct 20, 2023

Once the CI has completed.

@lesteve
Copy link
Member

lesteve commented Oct 20, 2023

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
============================= test session starts ==============================
platform linux -- Python 3.12.0, pytest-7.4.2, pluggy-1.3.0
rootdir: /home/vsts/work/tmp_folder
configfile: setup.cfg
plugins: forked-1.6.0, xdist-2.5.0, cov-4.1.0
gw0 I / gw1 I
gw0 [24972] / gw1 [24972]

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/_pytest/main.py", line 271, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>                          ^^^^^^^^^^^^^^^^^^^^^
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/_pytest/main.py", line 325, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/pluggy/_hooks.py", line 493, in __call__
INTERNALERROR>     return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
INTERNALERROR>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/pluggy/_manager.py", line 115, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
INTERNALERROR>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/pluggy/_callers.py", line 152, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>            ^^^^^^^^^^^^^^^^^^^^
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/pluggy/_result.py", line 114, in get_result
INTERNALERROR>     raise exc.with_traceback(exc.__traceback__)
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/pluggy/_callers.py", line 77, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>           ^^^^^^^^^^^^^^^^^^^^^^^^^
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/xdist/dsession.py", line 117, in pytest_runtestloop
INTERNALERROR>     self.loop_once()
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/xdist/dsession.py", line 140, in loop_once
INTERNALERROR>     call(**kwargs)
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/xdist/dsession.py", line 262, in worker_collectionfinish
INTERNALERROR>     self.sched.schedule()
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/xdist/scheduler/load.py", line 257, in schedule
INTERNALERROR>     self._send_tests(next(nodes), 1)
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/xdist/scheduler/load.py", line 269, in _send_tests
INTERNALERROR>     node.send_runtest_some(tests_per_node)
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/xdist/workermanage.py", line 284, in send_runtest_some
INTERNALERROR>     self.sendcommand("runtests", indices=indices)
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/xdist/workermanage.py", line 300, in sendcommand
INTERNALERROR>     self.channel.send((name, kwargs))
INTERNALERROR>   File "/usr/share/miniconda/envs/testvenv/lib/python3.12/site-packages/execnet/gateway_base.py", line 810, in send
INTERNALERROR>     raise OSError(f"cannot send to {self!r}")
INTERNALERROR> OSError: cannot send to <Channel id=1 closed>

@ogrisel
Copy link
Member Author

ogrisel commented Oct 20, 2023

Let me push this.

@ogrisel
Copy link
Member Author

ogrisel commented Oct 20, 2023

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:

https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=60198&view=logs&j=dfe99b15-50db-5d7b-b1e9-4105c42527cf&t=ef785ae2-496b-5b02-9f0e-07a6c3ab3081

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.

@ogrisel
Copy link
Member Author

ogrisel commented Oct 20, 2023

Yeah! the scipy_dev build is green with Python 3.11.

@lesteve
Copy link
Member

lesteve commented Oct 20, 2023

doc and cirrus arm were passing on 68ea16b

I pushed a logging tweak (which I noticed also in #27622).

Copy link
Member

@jjerphan jjerphan left a 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:

# 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?

@lesteve lesteve merged commit b22c706 into scikit-learn:main Oct 21, 2023
@jjerphan jjerphan deleted the ci-unpin-cython branch October 21, 2023 10:40
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2023
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants