-
Notifications
You must be signed in to change notification settings - Fork 447
Build wheels with cibuildwheel action #698
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
Conversation
| prune . | ||
| recursive-include pymoo *.py *.pyx *.pxd | ||
| recursive-include pymoo/cython/vendor *.cpp *.h *.py | ||
| include LICENSE Makefile pyproject.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LICENSE and pyproject.toml are included by default
| **/*.png | ||
| **/*.gif | ||
| **/*.cpp | ||
| pymoo/cython/*.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a cpp file under pymoo/cython/vendor that was also git-ignored prior to this change. We only want to ignore the cython-generated files.
|
@blankjul this PR is now reasonably sized, I think. A review would be most appreciated when you have the time, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just removing the executable bit on this file (it doesn't need to be executable, and no other source files here had it set)
| python-version: '3.10' | ||
| - name: Install Dependencies | ||
| run: | | ||
| pip install --no-cache-dir --upgrade setuptools>=77.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the build step in this workflow won't respect the bound in pyproject.toml, so a compatible version has to be installed by hand. This could be streamlined in a future PR.
|
Merged it to see how it runs in github actions |
| # --------------------------------------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| parser = argparse.ArgumentParser() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new setup is there a way to reproduce the compilation with the parameters below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm afraid I dropped it all when simplifying for my understanding! I had hoped to discuss before this was merged to better understand the original motivation.
Ideally it would be fantastic to get to the point that we ship binary wheels for all platforms that pymoo aims to support, and then we can remove the user-facing runtime fallback entirely. We'd keep the pure-python implementations of eg. fast-non-dominated sort for the test suite only.
This direction would imply that we'd always want to be able to fully build and compile the package, and that if there was a failure then that's a bug (either in our build setup, or in the environment of the person building it). It also has the nice side-effect of removing pymoo-specific customisation of setup.py, which makes it a little more approachable for new contributors (since it's more "vanilla").
(There's a separate issue of not running cythonize when building an sdist, which I experimented with in this PR but wasn't able to get working reliably. I don't think it's urgent, but I'll aim to get back to that in the future.)
| traceback.print_exc() | ||
| print("=" * 75) | ||
| print() | ||
| print("WARNING: For the compiled libraries numpy is required. Please make sure they are installed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a fallback to use non-compiled files if compilation failed. Does that still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aims to:
(See also https://github.com/tpgillam/pymoo/pull/1 for the equivalent PR into my own fork, for which actions have been run).
I'll add a few comments in-line on the PR. A lot more context and commentary is in #676, especially about the proposed migration to the "src layout": #691 (comment)
Probably the most signficant / controversial proposals here are:
the switch to src layoutadoption of uv for dependency & environment management, and as a build frontendedit: after a bit of debugging, I've got the wheel building working with the non-src-layout, which happily makes this PR a lot smaller. Latest wheel building GitHub action run here: https://github.com/tpgillam/pymoo/actions/runs/14481925309?pr=1
edit 2: the new github action uses uv internally to aid with building, but locally the previous workflow for building would still work (manually creating environment with appropriate build tools and invoking
python setup.pydirectly).