Skip to content

MNT bump to Cython=3.0.9 #28640

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 7 commits into from
Mar 19, 2024
Merged

MNT bump to Cython=3.0.9 #28640

merged 7 commits into from
Mar 19, 2024

Conversation

lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented Mar 16, 2024

Reference Issues/PRs

None

What does this implement/fix? Explain your changes.

Updates min required version of cython to 3.0.9.

Any other comments?

Copy link

github-actions bot commented Mar 16, 2024

✔️ Linting Passed

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

Generated for commit: 72c6eb0. Link to the linter CI: here

@ogrisel
Copy link
Member

ogrisel commented Mar 18, 2024

Out of curiosity, is this useful or needed for a specific PR/issue?

_min_dependencies.py should not be updated too often if not needed, even if we can rely on more recent versions for build dependencies, contrary to runtime dependencies.

I don't want to force contributors to always upgrade to the latest version of Cython if they don't need to.

EDIT: this change is mandated by #28603 although it's net yet obvious to me which part of this PR requires the Cython version bump.

@lesteve
Copy link
Member

lesteve commented Mar 18, 2024

Also side-comment, note that the nogil build needs some coordination work with Sam Gross, if we update the min Cython dependency, see for example colesbury/nogil-wheels#6.

For now the nogil lock-file is updated manually outside of the update_environment_and_lock_files.py script ...

@lorentzenchr
Copy link
Member Author

It‘s required for a bugfix of fused types used in a method of a cython.final extension type.

@ogrisel
Copy link
Member

ogrisel commented Mar 18, 2024

I resolved the conflict. Let me do a re-run of the update lock file script just to be sure.

If everything is green, I am fine with merging this version update if it makes it easier for other concurrent PRs on HGBDT.

@ogrisel
Copy link
Member

ogrisel commented Mar 18, 2024

Actually, let's wait for #28653 and then re-run the lock script on this PR.

EDIT: fixed PR number.

@ogrisel
Copy link
Member

ogrisel commented Mar 18, 2024

For reference, I opened the PR to build the Cython 3.0.9 wheel for nogil:

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.

I pushed a commit to trigger all the CIs with the new Cython version.

Assuming this is green, +1 on my side.

@ogrisel
Copy link
Member

ogrisel commented Mar 18, 2024

I opened #28654 to update the nogil CI separately.

@ogrisel
Copy link
Member

ogrisel commented Mar 18, 2024

#28555 was merged and some of the lock files need another update as a consequence. Let me push another commit.

@lorentzenchr
Copy link
Member Author

@ogrisel Thanks for taking over!

Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

LGTM

@lesteve lesteve merged commit a6718dd into scikit-learn:main Mar 19, 2024
@lesteve
Copy link
Member

lesteve commented Mar 19, 2024

Merging, thanks!

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