v4-CMake

[otbrown]

Hi @TysonRayJones,

Creating a PR to discuss and critique my work on the new build system :) I'll use this as a place to keep notes and thoughts too.

I've been through and identified the following open issues which I'll try to address, as well as my own biases:

• install prefix and library naming conventions #351 (Didn't know you could do that through CMake, but I like it!)
• QuEST/CMakeLists.txt broken on arm #389 and Illegal instruction when AVX instruction set is unsupported #259 - I should speak to Ali about this too. I have to say my gut feeling is "turn on -O3 a.k.a. Release and forget about it" is the optimal strategy these days, but I don't normally support such a range of environments...
• Inconsistent CMake naming #154 Suspect this one is deprecated!

Notes:

• I've bumped from 3.7 to 3.18 (which happens to be the version available in Debian 11 apt) but consider it a placeholder until the build system is complete, at which point I'll try to figure out a real minimum. I'd be very surprised if it's below 3.13, but might be willing to forgo 3.2x features...
• I personally dislike the build in place user executable functionality that was in the old CMake, but I'll make sure it continues to be supported alongside more a standard library build/install target.
• I will need support testing against QuESTLink/Windows things!

[TysonRayJones]

Wew sounds good! I'm happy to cop the Windows testing. I'll setup CI too once the build is runnable (and probably a linter, since I've already slopped in some spaghetti).

I agree it's clumsy/overbearing/amateurish to build the user's executable, though I suspect a good chunk of QuEST's user base have physics backgrounds and lack the requisite skills to make their own build, or integrate QuEST into a bigger stack. I want to make compilation as easy as possible for that group somehow. I'm open to better ideas!

[otbrown]

Can confirm that I can build and run a (very simple) example code using the available new API with and without multithreading enabled (though it doesn't deploy with MT at one qubit...), so now I can get started on the fun stuff.

Notes:

• Include paths need to be rationalised but this goes hand-in-hand with setting up the install targets, since we can use separate generators for the build and install includes.
• Once I've done that I can also reimplement the user executable functionality.
• Similarly, I can now look at install prefix and library naming conventions #351.
• On the other hand I can't make much headway with the GPU build(s) until tests are fixed.

[otbrown]

Bump to CMake 3.14 incoming for policy CMP0082.

[TysonRayJones]

Ahh I'm sorry for the rebasing and conflicting the PR! I hadn't realised the source files were being modified too. I'm intending to rename some files (structures to matrices since it' already quite long; other new data structures will have their own files) - is it okay if I rebase these too and we sort out the conflicts later? 😅

Btw are the relative path includes changed in b87b7f5 essential for CMake building? I was following Kolpackov's project structure (but preserving src and include subdirecs), and enjoyed the explicit separation (e.g. like in validation.cpp) of:

// API
#include "quest/include/modes.h"
#include "quest/include/environment.h"
#include "quest/include/qureg.h"
#include "quest/include/structures.h"

// core
#include "quest/src/core/errors.hpp"
#include "quest/src/core/bitwise.hpp"
#include "quest/src/core/memory.hpp"
#include "quest/src/core/utilities.hpp"
#include "quest/src/comm/comm_config.hpp"
#include "quest/src/cpu/cpu_config.hpp"
#include "quest/src/gpu/gpu_config.hpp"

// external library
#include <iostream>
#include <cstdlib>
#include <vector>
#include <map>

It was also consistent in that all paths were relative to the directory containing quest.

If I understand right, with the new includes, some are relative to include/ (fair enough)

#include "types.h"

and the rest are relative to the source file location (e.g. this in comm/):

#include "../core/errors.hpp"

This seems less elegant and consistent - is it necessary for compilation, or offer another benefit I've missed?

[otbrown]

No problem! Any conflicts should be pretty trivial to resolve, as I wouldn't expect to touch anything more than includes, so happy to just keep merging as and when.

Apologies, I'd assumed it was just an artifact of the make build 😅

Advantages of the way I've done it:

• Install directory has a standard structure: include, libs, bins. Users just need to add -I<installdir>/include to build their programs using the public API.
• Relative includes don't require a -I flag at all at compile time, and are slightly less sensitive to changes in the project structure.

However, through CMake, all things are possible 😉

• Build and install interfaces for include directories can be set separately so I can make CMake export whatever we like as as the install include interface, and we can just tell users how to correctly write their includes relative to that.
• Setting the build includes to anything is trivial and really doesn't affect users at all 🤷‍♂️

So very happy to change it all back, it's just a find/replace job! It's your project, so let me know which you'd prefer.

[otbrown]

Library renaming is now available -- the user can either just provide a custom string to produce lib${LIB_NAME}.so, or turn on VERBOSE_LIB_NAME to append to the lib name based on configuration options. This should all play nicely with the install export stuff too, though I haven't tested that.

GPU acceleration and MPI distribution are now also available, but there are two issues:

• cuQuantum: nvidia don't provide a CMake integration, and I don't realllllly want to write a findcuQuantum for them, though I will if I have to. Need to ponder that one.
• HIP: It looks as if the HIP version builds just by using the HIP compiler to compile the CUDA which is nice, but may present some issues for CMake. The simplest solution (for me) is to just split ENABLE_GPU_ACCELERATION into ENABLE_CUDA and ENABLE_HIP, but will see if there's a more creative solution! Never mind, there's already a USE_HIP option at play in v3, so I'll just make use of that! 😁

[otbrown]

Hi @TysonRayJones!

Hope you're well, and have found some time to enjoy a holiday in amongst the development work...

I've marked this as ready for review now, as I think what we have is enough to be useful for your planned 'developer' release, but naturally it's your call whether you'd like to merge now or later. Note the TODOs listed below. I've added docs/cmake.md to list the new CMake options.

I've got a paper to review, PhD thesis corrections to approve, and I'm getting married next week, so I probably won't be looking at this again before I'm back in the office 😅

Happy new year! 🎊

CMake TODO:

• Implement CuQuantum support. The CMake config variable exists but doesn't really do anything currently. As mentioned before this may require me to write a FindCuQuantum which should simple enough, but once I'm back in the office! This is the most egregious CMake specific issue now.
• What version of clang are you using? I stil get compilation failures on the tests with clang 16 and clang 18 I'm afraid. So I still need to test the tests, but all the required CMake is there I think.
• Test HIP support.
• Test compilation on Windows.
• Update compilation documentation, dependent on the tutorial program.

[TysonRayJones]

Wew amazing! Happy holidays to you, and a massive congratulations on your wedding!!! 🎉 🎉

I've got two items (🤞) left on my backlog before I'll start using your CMake build and test it on as many platforms as I can access.

To your dot points:

1. I recall having trouble linking cuStateVec with CMake in QuEST v3 because I was trying to support the despairingly old default CMake version on the Oxford cluster (as I lament here). I'm happy for us to force a modern CMake minimum version if it helps.
2. Oh my bad! I've been using only Clang 15 to run the v3 tests, i.e. test the deprecation API. It is probably that optional hacky header causing the problems - I'll check it out!
3. I'll have a go at compiling with HIP and on Windows, and can also update the revised tutorial doc - your args page looks brilliant!

Thanks very much for all your fantastic work so far! 🙌

[TysonRayJones]

Howdy! I've finally had time to use the CMake build. I get it to compile by...

1. copying over the tests folder from the scratch-v3-tests branch (which has some patches)
2. retaining your CMakeLists.txt from this PR (otherwise overwritten by above)
3. replacing the reference to DEAULT_VALIDATION_EPSILON (forgive the misspelling eep) in line 38 of test_utilities.hpp with a constant 1E-10.

Of course I'll fix these shenanigans!

Disabling the tests, I can indeed compile (on my Macbook) with both clang 15.0.0 and gcc 14.2.0 🎉

The tests do not compile (except in clang 15) due to deprecated.h's use of _Generic, like here. I've soberingly realised that this C11 syntax is not adopted by C++17 😢 (test here). Only a handful of compilers (like the Frankenstein's monster that is Clang) will accept it as an extension. Indeed with multithreading disabled (see below), I can successfully compile with clang 15.0.0, though as you've noted, even that may go away with newer clang versions!

So, let's just give up on cross-platform supporting the v3 unit tests, which seek only to test the deprecation API anyway. We'll instead target new, v4, C++17 compatible unit tests, which I'll prepare. The deprecation tests can run on old clang offline, since they're inessential ¯\(ツ)/¯

Some other notes:

• The doc suggests cmake -Bbuild . from the root dir, in lieu of the standard(?) cmake .. from within build. Not a problem, though we should note users should subsequently call make -C build to avoid confusion.
• Some additional bull shittery is needed to get OpenMP working with Clang compilers, shown here. We can direct Clang users to install libomp in the doc.

I channeled my _Generic frustrations inspired this meme:

[otbrown]

Thanks for taking the time to give it a whirl! Glad it's mostly working on someone else's computer.

It's a shame the deprecated API won't work, although reworking the tests probably is better in the long run.

To your notes:

• Good point! Or indeed using cd build && make. I can add this, no problem.
• Good spot! I was confused at first but no doubt I installed libomp-dev at some point not long after I last refreshed my VM 😅

It is weird (and a shame) that _Generic support was dropped 😞

I'm back in the office now, so I'll carve out some time around reviewing QCTiP submissions to test on our AMD GPU nodes and look into CuQuantum (finally!).

[TysonRayJones]

I've scaffolded the v4 unit tests here (commit) which is compiling on all my tested systems with the hacky bash script. No testing file needs an MPI compiler nor special preprocessors now 🎉 The tests temporarily use the super old Catch2 single-header, which we can trivially change to modern Catch2 when supported by the CMake build (replacing these stubs).

I'll hold off on developing the new tests until the CMake build is working, because use of the bash script is nausea-inducing. Should we try to merge your build into v4 (I suppose from a fresh fork)? Totally fine if unpolished; e.g. cuStateVec or Windows aren't working, which we can sort out in another commit (and defer HIP to v4.1 if necessary). I want to experiment with using CMake to activate integration tests based on what backends got compiled (without spilling more preprocessors everywhere) 🙌

(so feel free to completely ignore the integration/ folder - good chance it'll stay empty for v4)

I've been recruited by Zoë to similarly review some QCTiP submissions - truly it is the universal struggle ehehe

[otbrown]

Hi Tyson,

Nice! I've put together a CMake build for the new test structure for you. As ever it runs fine on my machine, but let me know if you encounter any issues! Would be happy to merge now and either leave this PR open, or I can open a new one to resolve remaining CMake issues.

Managed to get through 3 papers while we had an electrician in earlier, only 10 to go...

[TysonRayJones]

Oh! I brainfarted hard! The deprecation header contains only a single invocation of _Generic - the remaining uses I was remembering are actually in the v4 API, like matrices.h, which are guarded against C++ so work fine. Ergo I can easily tweak the deprecated v3 tests to work beyond clang 🎉 I'll get to it

[TysonRayJones]

Compiling and (well, non-distributed CPU) passing here with g++ 11 on Ubuntu 🎉

[TysonRayJones]

Nice! I've put together a CMake build for the new test structure for you. As ever it runs fine on my machine, but let me know if you encounter any issues! Would be happy to merge now and either leave this PR open, or I can open a new one to resolve remaining CMake issues.

Wew brilliant! There are some minor edits I'd nominate which I'll point out using the Github review system. Up to you how to continue edits after the squash-merge!

Managed to get through 3 papers while we had an electrician in earlier, only 10 to go...

Joy :) I'll be procrastinating mine right up until Friday it seems eheuheu

[otbrown]

After a bit of cajoling (i.e. making INCLUDE_DEPRECATED_FUNCTIONS a private compiler flag for the library, and realising that test_utilities were actually needed 😅), I now have a working CMake build of the deprecated tests too 🥳

I need to tidy it up, and probably make all compiler flags library private because that's probably good practise anyway (NOTE TO SELF: Add something to documentation so that users know that any QuEST flags are not inherited by their application), but should be able to push it up later this afternoon!

[TysonRayJones]

Wew lookin good! Some minor changes proposed

[TysonRayJones]

Feel free to move your name to the top! I suppose noting v4 is unnecessary since it will likely stay the build for future versions. E.g.

# author : Oliver Thomson Brown
# contributions to previous builds from:
# author : Ania Brown
# author : Jacob Wilkins
# author : Balint Koczor (Windows compatibility)
# author : Tyson Jones (testing)

Because this comment doesn't get parsed by doxygen, you can use any format you wish to make it prettier, e.g.

# authors : 
#   - Oliver Thomson Brown
#
# contributions to previous builds from:
#   - Ania Brown
#   - Jacob Wilkins
#   - Balint Koczor
#   - Tyson Jones

[TysonRayJones]

Do we have strong opinions on whether compiling of examples and tests should be ON by default? I like the default compilation (= the user's first experience) to compile the minimum possible, so that:

• the compilation is faster
• the console output is readable
• the chance for exotic platform-specific warnings or compilation errors is reduced
• the user has a stronger understanding of what's going on

I'd propose the default compilation to build a single C++ example file, which we could place in like examples/tutorials/main.cpp, and which compiles to put executable main in the root dir. A placeholder for now could be

#include "quest.h"

int main() {
    initQuESTEnv();
    reportQuESTEnv();

    Qureg qureg = createQureg(20);
    reportQuregParams(qureg);

    initPlusState(qureg);
    reportQureg(qureg);

    destroyQureg(qureg);
    finalizeQuESTEnv();
}

What'cha think?

[TysonRayJones]

(just noting these doc'd default options would need to be change if we do end up changing the build defaults)

[TysonRayJones]

ENABLE_DEPRECATION might not be super clear - one could imagine it "enables deprecation of old functions, switching to user the new ones" so ergo has the inverse of the intended meaning.

How about ENABLE_DEPRECATED_API? That makes it more explicit too that only the interface is changing, while the backend code is unchanged.

[TysonRayJones]

Might be worth adding:

This is also known as the target GPU's "compute capability" and can be discovered [here](https://developer.nvidia.com/cuda-gpus)

[TysonRayJones]

this comment/diff is no longer true/needed, banish it!

[TysonRayJones]

to the void ye scurvy outdated comment/call

[TysonRayJones]

This file should stay as main.cpp since I've dropped the test_ prefix convention of the v3 tests for the v4 ones. Happy to debate this though!

[TysonRayJones]

this file not needed! All utils are in utils/ now

[TysonRayJones]

this file not needed! All utils are in utils/ now

[TysonRayJones]

OH we were working at the same time heh, apologies if the review is not outdated!!

Also I've only just discovered I had missed your comments from 13th Sept since Github hid them behind the (imo unacceptably innocuous) "conversations hidden" button, apologies for not responding!!