Skip to content

Use docker image for nogil CI #23312

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

Closed
wants to merge 4 commits into from

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented May 9, 2022

Use the nogil/python docker image to avoid having to build Python nogil inside the CI. This should make the script a bit simpler.

Also it would make #22448 a bit easier to integrate Python nogil

@lesteve lesteve force-pushed the simpler-nogil-ci branch from aadfeb4 to 37fb867 Compare May 9, 2022 15:28
@lesteve lesteve marked this pull request as draft May 9, 2022 16:09
@thomasjpfan
Copy link
Member

It looks like Cython is failing to compile on docker. The version of Cython from the latest nightly run CI with nogil is the same as the one failing in this PR's docker image.

It could be that the docker image is out of date? According to docker hub, it was built one month ago, while there are commits in the nogil repo from 9 days ago.

@lesteve
Copy link
Member Author

lesteve commented May 9, 2022

Yep I naively thought the docker image was updated on each commit but I saw that it was not the case afterwards.

Not sure what the error is about, I can not reproduce locally so there may be some quirks with the CI environment ... I put this back in draft mode, this may not be worth the effort debugging it to be perfectly honest.

@lesteve
Copy link
Member Author

lesteve commented May 10, 2022

Interestingly the cython error disappeared on rerunning the CI ... I am going to close this since I have now included python nogil in #22448 and using the git clone is preferable to using a docker image that is updated every month.

@lesteve lesteve closed this May 10, 2022
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.

2 participants