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

Fix cibuildwheel with patch #131

Merged
merged 11 commits into from
Nov 3, 2022

Conversation

chrispap95
Copy link
Collaborator

This PR presents a way to unpin the cibuildwheel version by applying yet another patch in the fastjet-core. I am not very sure this is the right way to solve this so any input is very welcome.

There are two problems with updating cibuildwheel.

  1. The first one is rather simple. cibuildwheel introduced musllinux images in version 2.2.0 and these images don't have the yum package manager. For the moment, I disabled these wheels. Do we want to build them? If so then we need to pass a different [before-all]. I didn't try it.
  2. The second one is far more subtle and is with fastjet-core. After building the wheel locally and experimenting with the build, I realized that when invoking explicit_jets() and then closing the interactive python session, the segfault happens right then. I inserted debugging symbols and ran with valgrind and it pointed me to this line that is a warning message shown by fastjet when explicit_jets() is invoked. I commented it out and now everything works. I am still not sure why this is happening. Perhaps, some thread safety issue? Another hint is that this issue began when cibuildwheel updated python to 3.10.0 from 3.10.0rc2. I looked at the changelog but I didn't notice anything relevant.

@chrispap95
Copy link
Collaborator Author

By the way, if we decide this is the way to go, we might want to reimplement this warning ourselves before merging this.

@lgray
Copy link
Collaborator

lgray commented Nov 2, 2022

For sure updating is a very good idea, and until we get it figured out we should reimplement the patch ourselves and open a discussion with the fastjet authors to figure out a longer term solution. This looks like a fairly stable part of the code so patching shouldn't incur too much burden on us.

@henryiii
Copy link
Member

henryiii commented Nov 2, 2022

I'd recommend adding "cp311-*" to the build matrix and removing "cp36-*" (along with updating python_requires to drop 3.6 support!).

@lgray lgray enabled auto-merge (squash) November 2, 2022 21:37
@lgray lgray disabled auto-merge November 2, 2022 21:38
@lgray
Copy link
Collaborator

lgray commented Nov 2, 2022

@chrispap95 All that is required now is the python-side implementation of the exclusive_warnings that we've patched out for the time being. Then we can merge this and have quite a nice release on our hands. :-)

@chrispap95
Copy link
Collaborator Author

That shouldn't be too hard. I can give it a try! (unless you want to do it)

@lgray
Copy link
Collaborator

lgray commented Nov 2, 2022

All you!

@lgray
Copy link
Collaborator

lgray commented Nov 2, 2022

Now that I know about the boost version issue causing the failure, I can probably figure out how to get this working on conda as well.

@lgray
Copy link
Collaborator

lgray commented Nov 3, 2022

@jmduarte a review when you have time?

Copy link
Collaborator

@jmduarte jmduarte left a comment

Choose a reason for hiding this comment

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

Hi @chrispap95 @lgray it looks good to me! Two minor things:

  • Should we raise an issue on https://gitlab.com/fastjet/fastjet just in case they know of an easy fix for this error message segfault (e.g. placing it in a different place)?
  • Maybe we should rename the patches to patch_fastjet_i.txt and patch_clustersequence.txt or something just so we know what they do and don't have to rename them when we remove one of them soon.

@chrispap95
Copy link
Collaborator Author

  • Yes, renaming the patches makes sense. I will do it now.
  • As for raising an issue upstream, I think it is the right thing to do eventually. However, I haven't reproduced this using plain C++ (or even just swig). I am not sure if they will look into this at all unless we can reproduce it in a more traditional way.

@jmduarte jmduarte merged commit 9526332 into scikit-hep:main Nov 3, 2022
@jmduarte jmduarte linked an issue Nov 3, 2022 that may be closed by this pull request
@chrispap95 chrispap95 deleted the cibuildwheel-update2 branch November 3, 2022 21:25
@chrispap95 chrispap95 mentioned this pull request Nov 6, 2022
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.

CI wheel building fails for python 3.7+
4 participants