Skip to content
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

Generate wheel for Cython 3.0.8 #6

Closed
wants to merge 1 commit into from
Closed

Conversation

lesteve
Copy link
Contributor

@lesteve lesteve commented Feb 1, 2024

In scikit-learn we are testing nogil and we recently updated our minimum supported Cython version to 3.0.8 in scikit-learn/scikit-learn#28284.

Let's be optimist and update this action to see whether that works 🤞!

@lesteve
Copy link
Contributor Author

lesteve commented Feb 1, 2024

@colesbury so this action is trigger manually (workflow_dispatch), I am not sure whether there is a way to test it in this PR ...

More generally I am wondering if other people have showed interest for Cython 3 in nogil. There may be some known issues that I am not aware of.

@lesteve
Copy link
Contributor Author

lesteve commented Feb 20, 2024

@colesbury I have a modified version of Cython 3.0.8 that compiles. This modified Cython version can compile scikit-learn and the scikit-learn tests pass with nogil-3.9.

The diff compared to Cython 3.0.8 is quite small, since I guess some work has been upstreamed into Cython:
https://github.com/cython/cython/compare/3.0.x...lesteve:cython:3.0.8-nogil?expand=1

  • I cherry-picked two of your commits that still made sense on your 0.29.29-nogil tag and they moved some of the code to the right elif clause.
  • CYTHON_METH_FASTCALL needs to be disabled for Cython to compile.
  • I was not sure about CYTHON_PEP489_MULTI_PHASE_INIT so I kept the code as in your Cython fork

I am not sure what still needs to be done to turn it into a Cython 3 wheel in https://d1yxz45j0ypngg.cloudfront.net/cython/.

Of course, I completely understand if you have plenty of higher priority things to do!

If there is anything that I can help with, let me know!

@colesbury
Copy link
Owner

colesbury commented Feb 20, 2024

Thanks @lesteve. I added the CYTHON_METH_FASTCALL change to https://github.com/colesbury/cython/tree/3.0.8-nogil and a 3.0.x build file https://github.com/colesbury/nogil-wheels/blob/main/.github/workflows/cython-3.0.yml.

I started the wheel builds, but it looks like it will take the GitHub runners a little while to pick them up.

@lesteve
Copy link
Contributor Author

lesteve commented Feb 20, 2024

Looks like there are genuine issues in the Github Action, maybe this is fixed in cibuildwheel 2.16.5 as this seems similar to pypa/cibuildwheel#1740.

Error:


Run pipx run --python "C:\hostedtoolcache\windows\Python\3.11.8\x64\python.exe" --spec "D:\a\_actions\colesbury\cibuildwheel\nogil" cibuildwheel "." --output-dir '"wheelhouse"' --config-file '""' --only '""'
  pipx run --python "C:\hostedtoolcache\windows\Python\3.11.8\x64\python.exe" --spec "D:\a\_actions\colesbury\cibuildwheel\nogil" cibuildwheel "." --output-dir '"wheelhouse"' --config-file '""' --only '""'
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    VERSION: 3.0.8
    CIBW_BUILD: nogil39-*
    CIBW_ARCHS_LINUX: auto
    CIBW_ARCHS_MACOS: universal2
    CIBW_ARCHS_WINDOWS: AMD64
    CIBW_ENVIRONMENT: CFLAGS='-O3 -g0 -mtune=generic -pipe -fPIC' LDFLAGS='-fPIC'
Invalid --only='""', must be a build selector with a known platform
creating virtual environment...
creating virtual environment...
determining package name from 'D:\\a\\_actions\\colesbury\\cibuildwheel\\nogil'...
installing cibuildwheel from spec 'D:\\a\\_actions\\colesbury\\cibuildwheel\\nogil'...
Error: Process completed with exit code 1.

@colesbury
Copy link
Owner

Ah, thanks for the pointer!

@colesbury
Copy link
Owner

Ok, the Cython 3.0.8 wheel is uploaded. Please let me know if it works okay.

@lesteve
Copy link
Contributor Author

lesteve commented Feb 21, 2024

Great, thanks!

Seems like the scikit-learn nogil build is happy in scikit-learn/scikit-learn#28338 so I am happy too, let's close this one!

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.

None yet

2 participants