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

Wheels on all platforms with cibuildwheels? #28

Closed
ianhbell opened this issue Apr 23, 2024 · 7 comments
Closed

Wheels on all platforms with cibuildwheels? #28

ianhbell opened this issue Apr 23, 2024 · 7 comments

Comments

@ianhbell
Copy link
Contributor

My colleague pointed me to your repo. Would you like some pointers for compiling your library for all platforms/pythons and pushing to pypi with cibuildwheels? I've done that for a number of libraries and I think we could get that working pretty quickly.

Motivation: I'd like to use your repo to experiment with the RET calculations, and of course I could build from source, but also happy to help and be connected to other open-source developers in our field.

@vegardjervell
Copy link
Collaborator

Cool to hear that you're looking into RET!

That definitely sounds interesting! The current build system (if a couple bash scripts deserve to be called a "build system") definitely makes it a little bit of a hassle to distribute new wheels, which is the primary reason I haven't distributed wheels with more recent patches as often as I would like.

I'll take a look at cibuildwheels, but if you have time, don't hesitate to submit a PR :)

@ianhbell
Copy link
Contributor Author

ianhbell commented Apr 29, 2024

@vegardjervell Here are a few recommendations regarding pybind11 building in a clean cross-platform way (don't worry we'll still get to cibuildwheels, but one step at a time):

Would you like me to make the necessary updates to the build system? It will change your building process a bit but should make it more maintainable into the future.

Also, you can add methods to wrapped classes in pybind11 in a function instead of a macro. Again, easier to debug and maintain. Here is one example from deep in the commit history of teqp: https://github.com/usnistgov/teqp/blob/0179829d8bed48e4273d5b533901f504c0ac4526/interface/pybind11_wrapper.hpp#L21 (the new code is refactored to avoid this approach with an abstract base class)

@ianhbell
Copy link
Contributor Author

Here is the branch of teqp where I have been fiddling with scikit-build-core: usnistgov/teqp@main...scikit-build-core

@vegardjervell
Copy link
Collaborator

For the build system: If you have time to push a PR with updates for more fluid cross compilation and tighter integration with wheel, that would be great!

However, I should note that I have some code lying around for a standalone C++ module that I'm thinking of adding in here once I get some time on my hands. The pure C++ module will have some other dependencies that are not required with the current solution (specifically json and Eigen), and so I will be doing some work to ensure that the python-wrapped module can be compiled without requiring those dependencies.

In that sense, it may make more sense to update the build system a bit further in the future when I'm already going to need to do it.

Regarding the pybind11 wrapping: I'm painfully aware that the current solution is far from elegant, and is marked by being the first pybind11-wrapper I wrote. Cleaning it up is most definitely on my list of things to do, and is probably going to happen around the same time as when I get time to merge the standalone C++ module.

@ianhbell
Copy link
Contributor Author

ianhbell commented May 6, 2024

I'm almost done, will have a PR for you shortly. We might want to talk over the PR a little bit, I tried to have a light touch but had to make some changes in places. Also, the docs will need to be revised.

I had to disable the tests post-build in the action because I cannot find a version of thermopack that will a) make the tests pass on all platforms and b) is available on all platforms. Can you recommend one?

@ianhbell
Copy link
Contributor Author

ianhbell commented May 7, 2024

@vegardjervell
Copy link
Collaborator

Resolved by #32. Building for linux will be enabled as soon as we distribute manylinux2014 wheels for ThermoPack v2.2, as cibuildwheels seemed to have trouble identifying the ThermoPack v2.2 wheel currently on pypi.

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 a pull request may close this issue.

2 participants