Remove superbuild #43

robertodr · 2023-05-29T09:44:45Z

See discussion here conda-forge/cppe-feedstock#22

Remove the local copy of Eigen. Dependency can be satisfied very easily through package managers, including conda.
Remove the auto-download of pybind11 if not present. Dependency can be satisfied very easily through package managers, including conda. In pyproject.toml it can be in the requires of the build system, so that pip itself figures out how to satisfy the dependency.
Reorganize CMake so as to remove the superbuild.
Refactor/update GitHub actions set up.
Switch setup.py to use scikit-build. git grep returns mentions of scikit-build but I can't see it used.

robertodr · 2023-05-30T05:46:20Z

@maxscheurer current status:

I've overhauled CI for Linux and macOS. I had to fork the polarizationsolver package.
Need to add Windows in the CI as well.
Same for the publish.yml action, I think I can use cibuildwheel there.

Questions:

I've had to remove the possibility to build libcppe as static library. Is that strictly necessary to have?
Is distribution of a C++ library needed? Or is the project used exclusively as a Python module? If the latter, I can simplify the CMake further.

maxscheurer · 2023-05-31T07:15:15Z

This is a rather enormous change (if I understand correctly pip install . wil be the only way to build the Python module after the change)

I think the enormous change is probably not worth it for now, unless it'll make Win stuff easier?

@loriab any hints on making the tests run on Win? (See comments above)

robertodr · 2023-05-31T07:30:16Z

It might make the windows stuff easier, but I think it's really within reach as things stand right now. Also, the Windows conda package builds, so we can skip running tests here for now, make a new release, and revisit later.

maxscheurer · 2023-05-31T20:22:15Z

What if instead of building w/ CMake, we run pip install . directly? Or adding the cmake install step? Would that maybe fix the Windows problem? 🙈
Thanks already for this enormous amount of very tedious work you put in! 🤩

loriab · 2023-05-31T20:41:11Z

On the Windows testing, it doesn't look like you're installing cppe anymore, and I wonder if that might help. There's only one mention of the pycppe*pyd file in the logs, and I'm not sure if it makes it there. Also, https://github.com/maxscheurer/cppe/pull/43/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR87 PYTHONPATH adjustment is Unix-like with the version.

robertodr · 2023-06-01T05:38:31Z

Indeed the Python package ends up in Lib\site-packages\cppe on Windows. I changed that, but it didn't help. However, __init__.py is symlinked and I think that's what messes windows up. It's a proper file when installing, so that's what I'll try next Thanks Lori! 👍

robertodr · 2023-06-01T13:04:16Z

OK, now tests run and pass on all 3 platforms. On Windows, the failure was a combination of 4 things:

The location where Python modules are installed is Lib\site-packages without the python<major>.<minor> bit.
The PYTHONPATH is empty and appending to an empty env-var is apparently generating invalid env-vars.
There was another pytest lying around, so simply running pytest didn't honor the PYTHONPATH. python -m pytest fixed it (I make this mistake all the time and still haven't learnt from it 🤦🏻)
Mismatch in linked-in OpenMP library on Windows. This requires setting KMP_DUPLICATE_LIB_OK=TRUE before running the tests.

Now I have to modify the setup.py to finish this up.

loriab

Nothing important here -- just thought I'd read through and see what other projects are doing.

loriab · 2023-06-01T14:04:09Z

.github/workflows/ci.yml

+        environment-file: .github/environment.yml
+        channel-priority: strict
+        python-version: ${{ matrix.python-version }}
+        use-only-tar-bz2: true # IMPORTANT: This needs to be set for caching to work properly!


You risk running out of packages soon and having inexplicable can't-solve environments with this since c-f is only serving up .conda packages now.

Yeah, I've seen that somewhere else. I just wanted to get caching to work and it seems that tar-bz2 option is needed for that.

loriab · 2023-06-01T14:08:07Z

.github/workflows/ci.yml

+           python -m pytest --capture=no --log-cli-level=INFO --pyargs cppe
+        else
+           export PYTHONPATH=$PYTHONPATH:Software/lib/python${{ matrix.python-version }}/site-packages
+           python -m pytest --capture=no --log-cli-level=INFO --pyargs cppe


Congratulations on your hard-won pytest line :-)

Adding --color yes makes it prettier.

loriab · 2023-06-01T14:13:12Z

cmake/compiler_flags/CXXFlags.cmake

+  if(CMAKE_CXX_COMPILER_ID MATCHES Intel)
+    set(ARCH_FLAG "-xHost")
+  endif()
+endif()


in case it's handy, here's a modernized xhost set: https://github.com/jturney/ambit/blob/master/cmake/xhost.cmake . it gets combined with https://github.com/jturney/ambit/blob/master/CMakeLists.txt#L68

loriab · 2023-06-01T14:33:13Z

.github/workflows/ci.yml

+        auto-update-conda: true
+        auto-activate-base: false
+        activate-environment: cppe-gha
+        environment-file: .github/environment.yml


If it ever starts being painful to solve, I'm a fan of mamba in CI: https://github.com/psi4/psi4/blob/master/.github/workflows/ecosystem.yml#L255-L256

I tried mamba actually, but for some reason it failed to solve 🤔

But i think it's because of my insistence on tar.bz2. I'll bust the cache and retry without that option. Let's see. Maybe I get the caching cake and I eat it too with mamba

loriab · 2023-06-01T14:49:10Z

.github/workflows/ci.yml

+        cd
+
+        if [ "${{ matrix.os }}" == "windows-latest" ]; then
+           export KMP_DUPLICATE_LIB_OK=TRUE 


If anyone knows or reads a good explanation of the threading situation on Windows, please forward. I've avoided the KMP_DUPLICATE route for psi4 by using Intel OpenMP and the build solution inherited from Raimondas, https://github.com/conda-forge/psi4-feedstock/blob/main/recipe/meta.yaml#L66-L77 . More library and compiler confusion here https://gitlab.kitware.com/cmake/cmake/-/issues/23241#note_1145997 .

I went on the (lazy) path of least resistance and used the KMP_DUPLICATE_LIB_OK hack... Maybe the solution by Raimondas is the best thing to do if problems arise?

The principle of what Raimondas did seems sound, but hiding away an old import lib at GH to link against is decidedly weird -- I'm a little shocked it made it through staged-recipes review. My path of least resistance was to take trouble at the build stage and avert showing KMP_DUPLICATE weakness to the user. :-) No idea which is better -- I'm just fishing for guidance. I'll let you know if I hear of any trouble with the Raimondas route.

cppe/cmake/cppeConfig.cmake.in

CMakeLists.txt

maxscheurer · 2023-06-01T16:36:54Z

I guess other than that, just the PyPI pipeline needs to be refactored, but I can take care of that in a separate PR.

robertodr · 2023-06-01T16:40:37Z

Yes, you can squash-merge this. If you're willing to wait a couple more days I can do the overhaul of setup.py and the publish.yml workflow.

maxscheurer · 2023-06-01T16:42:28Z

Sure, that'd be great 👍🏻

maxscheurer

🚀

robertodr added 8 commits May 29, 2023 09:35

Remove local copy of Eigen

52a8ed3

Remove autodownload CMake script for pybind11

46de81f

Clean up CMake local modules folder

2df4bad

CXX flags handling, remove some more CMake modules

ecf7f59

Remove superbuild pattern

5513d78

Send RUNTIME to BINDIR

63c6713

Fix installation of targets

583e3ce

Forgot to move the templated config file

68879d3

robertodr mentioned this pull request May 29, 2023

Add windows conda-forge/cppe-feedstock#22

Merged

5 tasks

robertodr added 17 commits May 29, 2023 14:58

Fix a couple of warnings

1e99195

Add target_link_options -undefined dynamic_lookup on mac

24eeba6

Workflows

74609ed

Drop numba (doesn't seem to be used anywhere)

1703700

SIlly commit

dac224d

Run on all branches

a367b94

Fix environment file

db62c00

Fix environment file

88aad6d

Fix versioning of polarizationsolver

006331d

Print cmake version

3fff542

Remove use of OBJECT library

08c75df

Build shared

fb33424

Fix RPATH handling, pybind11 options

b6812fb

Shared lib only, use OBJECT library

91e93e3

Fight with clang-cl

326be3b

uff

f3a6d4c

Give up on windows for a moment

4fabd78

robertodr and others added 3 commits May 30, 2023 07:51

Update ci.yml

e27c0f1

retry windows

d5effbd

retry windows

16631d7

add markers in conftest.py

2a00320

robertodr added 2 commits June 1, 2023 07:22

site-packages on windows is not versioned!

7e27298

remove non-existent CMake flag

1749cf8

robertodr added 11 commits June 1, 2023 11:38

use install path

a940578

use install path

06948be

use install path

03f0110

use install path

cef6b00

use install path

ad04d67

use install path

cfd2c90

use install path

a87dd9a

enable caching of conda packages

3c24eab

test run!

f9444d1

no mamba

5453c72

dammit

416ad16

loriab reviewed Jun 1, 2023

View reviewed changes

Suggestions from Lori

0b4954d

robertodr marked this pull request as ready for review June 1, 2023 16:40

maxscheurer approved these changes Jun 2, 2023

View reviewed changes

maxscheurer merged commit 37d49bb into maxscheurer:master Jun 2, 2023

robertodr deleted the patches-windows branch June 2, 2023 07:01

loriab mentioned this pull request Sep 25, 2023

Rebuild for python312 conda-forge/cppe-feedstock#23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove superbuild #43

Remove superbuild #43

robertodr commented May 29, 2023 •

edited

Loading

robertodr commented May 30, 2023

maxscheurer commented May 31, 2023

robertodr commented May 31, 2023

maxscheurer commented May 31, 2023

loriab commented May 31, 2023

robertodr commented Jun 1, 2023

robertodr commented Jun 1, 2023

loriab left a comment

loriab Jun 1, 2023

robertodr Jun 1, 2023

loriab Jun 1, 2023

loriab Jun 1, 2023

loriab Jun 1, 2023

robertodr Jun 1, 2023

loriab Jun 1, 2023

robertodr Jun 1, 2023

loriab Jun 1, 2023

robertodr Jun 1, 2023

loriab Jun 1, 2023

maxscheurer commented Jun 1, 2023

robertodr commented Jun 1, 2023

maxscheurer commented Jun 1, 2023

maxscheurer left a comment

Remove superbuild #43

Remove superbuild #43

Conversation

robertodr commented May 29, 2023 • edited Loading

robertodr commented May 30, 2023

maxscheurer commented May 31, 2023

robertodr commented May 31, 2023

maxscheurer commented May 31, 2023

loriab commented May 31, 2023

robertodr commented Jun 1, 2023

robertodr commented Jun 1, 2023

loriab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxscheurer commented Jun 1, 2023

robertodr commented Jun 1, 2023

maxscheurer commented Jun 1, 2023

maxscheurer left a comment

Choose a reason for hiding this comment

robertodr commented May 29, 2023 •

edited

Loading