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

cmake build #157

Merged
merged 30 commits into from
Sep 19, 2023
Merged

cmake build #157

merged 30 commits into from
Sep 19, 2023

Conversation

kleinhenz
Copy link
Contributor

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 deprecated numpy.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, using numpy.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 specify cmake as a build dependency so it will be fetched from pypi 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.

@mpmdean
Copy link
Contributor

mpmdean commented Mar 10, 2022

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:

  • There are a couple of small linter issues to fix
  • docs/source/installation.rst would need an update

@kleinhenz
Copy link
Contributor Author

fixed the ci issues. happy to update the docs if you decide you are happy with this approach and would like to merge.

@mpmdean
Copy link
Contributor

mpmdean commented Mar 16, 2022

@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.

@mpmdean
Copy link
Contributor

mpmdean commented Mar 17, 2022

@kleinhenz

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 docs/source/installation.rst. Thanks!

@mrakitin mrakitin self-requested a review March 18, 2022 17:53
@kleinhenz
Copy link
Contributor Author

I've updated the linux source installation instructions in docs/source/user/installation.rst. I bumped the ubuntu version to 20.04 which is the current LTS. I checked that this worked for me starting from scratch on ubuntu 20.04 and installing everything using apt-get. Let me know if you run into any more issues trying to install!

@mpmdean
Copy link
Contributor

mpmdean commented Mar 19, 2022

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.

@mpmdean mpmdean mentioned this pull request Nov 22, 2022
@mpmdean mpmdean mentioned this pull request Jun 27, 2023
@mpmdean
Copy link
Contributor

mpmdean commented Sep 15, 2023

Hi @mrakitin is it better to build the wheel as follows

python -m pip wheel .

@mrakitin
Copy link
Member

Not sure why the python setup.py bdist_wheel command failed in the CI, but it worked for me locally (at least started).

Please feel free to update the command and rerun the jobs.

@mpmdean
Copy link
Contributor

mpmdean commented Sep 16, 2023

Looks like python -m pip wheel . works.

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.

Copy link
Member

@mrakitin mrakitin left a 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:

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)
Copy link
Member

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.

@mrakitin mrakitin merged commit 252774e into NSLS-II:master Sep 19, 2023
14 checks passed
@mrakitin mrakitin mentioned this pull request Sep 19, 2023
2 tasks
@mpmdean
Copy link
Contributor

mpmdean commented Sep 19, 2023

Hey @Anselmoo do you consider this fixed by #157 ?

@Anselmoo
Copy link
Contributor

Sorry @mpmdean, I might be a little bit too early.

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.

4 participants