search

thermotools / KineticGas

Implementation of the Revised Enskog Theory for Mie fluids (RET-Mie) for computation of diffusion coefficients, thermal diffusion coefficients, viscosity and thermal conductivity
MIT License
3 stars 2 forks source link

Wheels on all platforms with cibuildwheels? #28

Open ianhbell opened 3 weeks ago

ianhbell commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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

vegardjervell commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

Build results are here: https://github.com/ianhbell/KineticGas/actions/workflows/cibuildwheel.yml