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

Enable binary wheels for all python versions and OS #30

Closed
wants to merge 29 commits into from

Conversation

ianhbell
Copy link
Contributor

@ianhbell ianhbell commented May 7, 2024

I just jumped ahead and went right to scikit-build-wheels. We should test that the wheels all work, once we figure out a reasonable version of thermopack.

See #28

@ianhbell
Copy link
Contributor Author

ianhbell commented May 7, 2024

Probably we should set up a time to chat about this PR. Shoot me an email at ian.bell@nist.gov and we can discuss a few things.

CMakeLists.txt Outdated
# set(CMAKE_CXX_COMPILER /usr/bin/g++) # Setting these can, on some systems, cause cmake to go into an infinite loop (see: build.sh to set the compiler)
# set(CMAKE_C_COMPILER /usr/bin/gcc) # Setting these can, on some systems, cause cmake to go into an infinite loop (see: build.sh to set the compiler)
if(APPLE)
set(CMAKE_CXX_FLAGS "-Wall -Wextra -Wno-unknown-pragmas -Wno-sign-compare -pthread -arch arm64")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops this should be fixed. The arch should not be specified this way. It should instead be something like: https://github.com/usnistgov/teqp/blob/201ceb7c59780bd93df484b90c2b2e986823116f/CMakeLists.txt#L2

@vegardjervell
Copy link
Collaborator

Nice stuff! I'll have a closer look at it in the coming couple of days, then we can resolve any final tweaks :)

@vegardjervell vegardjervell linked an issue May 7, 2024 that may be closed by this pull request
@ianhbell
Copy link
Contributor Author

ianhbell commented May 7, 2024

Aside: it is better to leave the name the same in debug and release builds. Then you can just flag to build one or the other.

@vegardjervell
Copy link
Collaborator

I agree, what you've caught there are some more leftovers from early development that was never cleaned out. I've had a list of to-do's in cleaning up and/or streamlining quite a bit of stuff (pybind11 bindings, the integration module for collision integrals, the factorial module, etc.) which I try to find time for every now and then.

@ianhbell
Copy link
Contributor Author

ianhbell commented May 7, 2024

I agree, what you've caught there are some more leftovers from early development that was never cleaned out. I've had a list of to-do's in cleaning up and/or streamlining quite a bit of stuff (pybind11 bindings, the integration module for collision integrals, the factorial module, etc.) which I try to find time for every now and then.

Yup, that's the way these things go. We'll get there. The sticking point right now is a version of thermopack that will work ok on all platforms and python versions. I guess they are not doing cibuildwheels for that either; I had volunteered to help with that some time back.

@vegardjervell
Copy link
Collaborator

vegardjervell commented Jun 21, 2024

I've had a little bit of time to look at this now, the only issue I can see is that find_package(pybind11 REQUIRED) doesn't appear to be very robust. I've had a hard time helping cmake find the package without doing a global install of pybind11, which I would definitely prefer to avoid.

As of now, this package is (to my knowlege) far more commonly compiled from source for consumption rather than distribution. The way I see it, making sure it is easy for others than the lead devs to compile from source is more important than streamlining the distribution builds.

In the same line, the fnd_package(Python) statement should somehow be modified to prefer the python version currently pointed to by which python such that the correct version is built when a venv is activated, as was previously solved in the build script by passing a the default value -DPYTHON_EXECUTABLE=$(which python) to cmake.

@ianhbell
Copy link
Contributor Author

You shouldn't need to do anything magic with the python executable, it should be found properly, automatically. I know that works for me almost always and I use conda environments heavily, and I should hope the same thing would work properly with venv. For pybind11, I typically use it as a git submodule because I don't like global installs either.

Let's plan to sit down together in Boulder and go over the details.

@vegardjervell
Copy link
Collaborator

For some reason cmake insisted on using a different python version than what I was using in my venv unless I specified set(Python_FIND_VIRTUALENV ONLY). I've made some modifications to make the project build for me on this branch. I also had some issues with the binary not being included when I pip install the project. With setup.py I've just used find_packages to include it, but I don't have much experience with pyproject.toml (even though I should probably get up to speed on that anyway).

@ianhbell
Copy link
Contributor Author

Hrm. Usually that "just works" but to be honest I don't use venv, I always use conda environments and I think the scientific python folks do too. I would make the leap to scikit-build-core, I have been using that extensively in a new library of mine and been happy with it, allows for a very responsive build system and easy debugging.

@vegardjervell
Copy link
Collaborator

I've figured out of the remaining issues and put them up on #32, so I'll close this. In case anyone comes looking here I'll mention that the major problem was that files ignored by .gitignore were automatically ignored when the wheel was being built, so libpykingas needed to be explicitly included in the wheel by adding

[tool.scikit-build]
sdist.include = ["libpykingas*"]

to pyproject.toml. Now that I've been looking a bit at this, I've noticed that theres one thing I really wish I had: A way of testing the whole system locally. Do you know if github offers any way to download the images it uses for its runners and a way to set up a local container to test the build system? I tried act quickly, but it didn't play very nicely with macOS on arm64.

@vegardjervell vegardjervell mentioned this pull request Jul 26, 2024
@ianhbell
Copy link
Contributor Author

I've figured out of the remaining issues and put them up on #32, so I'll close this. In case anyone comes looking here I'll mention that the major problem was that files ignored by .gitignore were automatically ignored when the wheel was being built, so libpykingas needed to be explicitly included in the wheel by adding

[tool.scikit-build]
sdist.include = ["libpykingas*"]

to pyproject.toml. Now that I've been looking a bit at this, I've noticed that theres one thing I really wish I had: A way of testing the whole system locally. Do you know if github offers any way to download the images it uses for its runners and a way to set up a local container to test the build system? I tried act quickly, but it didn't play very nicely with macOS on arm64.

I also looked into act because I had the same problem and I recall that I couldn't get it working, but I don't remember why.

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.

Wheels on all platforms with cibuildwheels?
2 participants