-
Notifications
You must be signed in to change notification settings - Fork 20
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
cmake build #157
cmake build #157
Conversation
ARPACK::PARPACK -> PARPACK::PARPACK
* enables fetching cmake from pypi so we don't have to worry about problems with out of date cmake versions
* use pip build * get mpi + openblas + python from ubuntu * use venv instead of conda
Thanks very much @kleinhenz ! I would agree the previous build method is a pain. I, and some others more knowledgeable than me, need to take a look over this, but I feel that it is very likely worth including this change after some careful review. A couple of quick thoughts would be:
|
fixed the ci issues. happy to update the docs if you decide you are happy with this approach and would like to merge. |
@kleinhenz Just a quick note that as well as this build procedure improvement we also have some issues with our conda feedstock. Our plan, led by @mrakitin who is infinitely more knowledgeable than me about these matters, is to first fix some problems with our conda feedstock and then to come back to review this. |
I had a look at installing this branch. The docker build went fine for me. I also tried to install it from scratch into a ubuntu virtual machine, but I failed. Since I'm not very knowledgeable about complex package dependencies my mistake is probably just a stupid one and not really worth debugnig, could you write what would need to be installed? Either here or in |
I've updated the linux source installation instructions in |
Using the revised instructions, I successfully installed this branch into an ubuntu virtual machine. So it's passing its "idiot-test" :) I had a lot of trouble with the prior version. |
Hi @mrakitin is it better to build the wheel as follows
|
Not sure why the Please feel free to update the command and rerun the jobs. |
Looks like I will also release the pin on the setuptools version, which was only in put in place due to the distutils issues that we are fixing here. |
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.
Looks good to me!
Some notes for the future work before releasing the next version:
- consider switching to the
python -m build
approach, per https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html (optional for now, need to rework the structure to work solely withpyproject.toml
file and maybe meson) - merge the
docs-publish.yml
workflow withci-test.yml
by adding a conditional step to publish on release - add a PyPI publishing job similar to https://github.com/NSLS-II/sirepo-bluesky/blob/main/.github/workflows/publish-pypi.yml
I propose we do those changes via a separate PR to avoid growing this PR further.
|
||
if (EDRIXS_PY_INTERFACE) | ||
message(STATUS "Building python interface") | ||
find_package(Python3 3.7 REQUIRED COMPONENTS Interpreter Development NumPy) |
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.
I wonder if it will only find Python 3.7 in this case... I guess we can keep it for now and see what happens when we remove py37 support.
Sorry @mpmdean, I might be a little bit too early. |
This changes the build system to use cmake and allows the full build to be driven by setuptools so that installation is reduced to a single
pip install
command (provided all dependencies are found). This replaces use of the deprecatednumpy.distutils
module to build the python extension and follows the patterns used in other projects e.g. pyscf.The main advantage is removing the IMHO difficult configuration of
numpy.disutils.core.Extension
(which is being removed in the future). Anecdotally, usingnumpy.distutils
on cori with the cray compiler wrappers is a giant pain. Additionally this reduces the number of steps to install, removes the necessity of manually editing the makefile to specify how the libraries should be linked, and gives you out of tree builds for free.pyproject.toml
is used to specifycmake
as a build dependency so it will be fetched frompypi
at build time so there is no need to worry about the user having cmake installed.This also updates the docker image to use the new build method and to use multistage builds which remove the need to chain everything into a single command.
I've successfully used this to build on opensuse, ubuntu (docker), and on cori.
If this is too disruptive obviously feel free to ignore. I mostly did this to make it more convenient for me to use.