Skip to content

CI Use development Cython wheel rather than building from source #29223

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 4 commits into from
Jun 11, 2024

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Jun 10, 2024

Cython has a development wheel in scientific-python-nightly-wheels as mentioned by @betatim in #29191 (comment)

@lesteve lesteve changed the title CI Use development Cython wheel rather than buildind from source CI Use development Cython wheel rather than building from source Jun 10, 2024
Copy link

github-actions bot commented Jun 10, 2024

✔️ Linting Passed

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

Generated for commit: 47fdbaa. Link to the linter CI: here

@@ -55,7 +55,7 @@ pre_python_environment_install() {
check_packages_dev_version() {
for package in $@; do
package_version=$(python -c "import $package; print($package.__version__)")
if ! [[ $package_version =~ "dev" ]]; then
if [[ $package_version =~ "\.[0-9]+$" ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the logic here. Shouldn't \. be under the scope of the + operator?

Also why a $ without an ^?

I would have expected:

Suggested change
if [[ $package_version =~ "\.[0-9]+$" ]]; then
if [[ $package_version =~ "^[\.0-9]+$" ]]; then

I confirmed it with https://regex101.com/r/O54PWi/1

Copy link
Member Author

@lesteve lesteve Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was only checking the last part after the dot, dot included, i.e. .0a0 in 3.1.0a0. I agree that your regex is more explicit. I'll push a fix doing this with the simplification that you can use . instead of \. inside [].

Side-comment: the reason that this check needed to change is that Cython development version does not have dev in its name and is actually 3.1.0a0.

@ogrisel
Copy link
Member

ogrisel commented Jun 10, 2024

Apart from the regexp discussion above, this LGTM. I checked the CI logs for the [scipy-dev] and [free-threading] runs and both use Cython 3.1.0a0 and other dev dependencies.

@ogrisel
Copy link
Member

ogrisel commented Jun 10, 2024

Please re-trigger [scipy-dev] to check that the regexp works as expected.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming green CI on a new [scipy-dev] build.

@lesteve lesteve added the Quick Review For PRs that are quick to review label Jun 11, 2024
@lesteve
Copy link
Member Author

lesteve commented Jun 11, 2024

The free-threaded and scipy-dev CI build are green and I double-checked that Cython development version was used in both builds, merging!

@lesteve lesteve merged commit 7e8ad63 into scikit-learn:main Jun 11, 2024
33 checks passed
@lesteve lesteve deleted the use-cython-dev-wheel branch June 11, 2024 13:39
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
11 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build / CI Quick Review For PRs that are quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants