-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
CI Use latest Cython sources in scipy-dev build #25300
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
Since we already do the same for |
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 given that the CI passes.
Thank you, @lesteve.
Turns out there are plenty of compilation errors with latest Cython development version 😢. See https://dev.azure.com/scikit-learn/d9c6f05f-7f58-4a17-b143-2a4e7ff015af/_build/results?buildId=50541&view=logs&jobId=dfe99b15-50db-5d7b-b1e9-4105c42527cf. I'll try to have a closer look at one point. |
It seems that we were relying on some casting that are not allowed anymore ;) |
And we have some new warnings :) (seems to be about the |
Starting Cython 3.0, exceptions will be propagated by default to Python if C interfaces are not Still, cython/cython#5094 adds a flag to be able to keeps the previous behavior, easing the migrating to Cython 3.0 (for some context, see cython/cython#5088). |
The scipy-dev failures are the same as in main, see https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=50527&view=logs&j=dfe99b15-50db-5d7b-b1e9-4105c42527cf&t=ef785ae2-496b-5b02-9f0e-07a6c3ab3081. |
Not sure whether the This program does not compile with latest development Cython: cdef int exponent = 2
cdef int result
result = 2 ** exponent with the following error:
This compiles fine with Cython latest release (0.29.32) |
From cython/cython#5016:
|
Nice thanks, it still seems a bit weird that the float is not cast to int automatically. Do you agree? In other words, in the following snippet cdef int exponent = 2
cdef int ivalue
cdef fvalue = 2.
ivalue = fvalue
ivalue = 2 ** exponent |
I don't know what the default type of a I guess not allowing doubles/floats to be assigned to an |
Oops indeed I forgot the OK let's say it kind of makes sense then. |
Not sure about the linux-arm64 failure a ConvergenceWarning in This does not seem related to the changes in this PR. I restarted the CI to see if this still exists. Full error
|
The linux-arm64 error did not happen this time 🤷 |
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.
Thank you for the PR!
} | ||
|
||
if not Cython.__version__.startswith("0."): | ||
compiler_directives["legacy_implicit_noexcept"] = True |
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.
Given that this flag will be removed in the future, may we include a TODO
comment here that links to cython/cython#5088 and states that we need to fix this?
sklearn/_build_utils/__init__.py
Outdated
"cdivision": True, | ||
} | ||
|
||
if not Cython.__version__.startswith("0."): |
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.
This setting will not allow scikit-learn to build with any of Cython's alpha releases because legacy_implicit_noexcept
has not been released in any of the alphas. I wanted to suggestion this:
if parse(Cython.__version__) >= parse("3.0.0a11"):
But the dev branch of Cython is still "3.0.0a11": https://github.com/cython/cython/blob/master/Cython/Shadow.py#L4, which is the same the most recent PyPi alpha release.
I am okay with the compromise of only allowing scikit-learn to build with Cython 3 on the dev branch.
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.
FYI, I have opened cython/cython#5204 to ask for a support of Development release segment in public version identifiers as specified by PEP 440.
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.
Now that cython/cython#5204 has been addressed, we should be able to use this:
if not Cython.__version__.startswith("0."): | |
if not Cython.__version__.startswith("3.0.0a12.dev0"): |
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.
Your PR was likely merged too quickly and broke installing cython from the master branch, see cython/cython#5204 (comment)
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.
cython/cython#5208 should fix that.
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.
One last comment. This is still mergeable to me if the CI jobs pass.
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
sklearn/_build_utils/__init__.py
Outdated
# TODO: once Cython 3 is released and we require Cython>=3 we should get | ||
# rid of the `legacy_implicit_noexcept` directive. | ||
# This should mostly consist in: | ||
# |
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.
According to the [linting job]https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=50782&view=logs&jobId=32e2e1bb-a28f-5b18-6cfc-3f01273f5609), there is a black formatting issue here.
What does this implement/fix? Explain your changes.
With Cython 3.0 approaching it would be great to test scipy-dev against latest Cython rather than some alpha that we don't really know what it corresponds to. For example latest Cython alpha is from July 31 2022 https://pypi.org/project/Cython/#history.
Any other comments?
This adds 2-3 minutes to the build, I don't think this is an issue since this is mostly run in scheduled builds and some specific PRs.