Enable user to take ownership of MPI#722
Conversation
…unction name to comm_getMpiComm
…ing for subcomm compiled
…for SubComm, because of course it enforces CXX
…declare that they take ownership of MPI
… updated setComm for init only workflow
…ything but it makes MPI library implementers less nervous
…ith user owned MPI
|
On the plus side, a distributed QFT benchmark circuit runs perfectly correctly: |
| */ | ||
| void initCustomQuESTEnv(int useDistrib, int useGpuAccel, int useMultithread); | ||
|
|
||
| void initCustomMpiQuESTEnv(int useDistrib, int userOwnsMpi, int useGpuAccel, int useMultithread); |
There was a problem hiding this comment.
Do we not want a new initialiser so its really clear where there is a different setup? Also this probably braked compatibility for older software.
There was a problem hiding this comment.
That is a new initialiser! This is effectively a C interface, so no overloading allowed.
There was a problem hiding this comment.
Apologies I had missread this I thought it was replacing initCustomQueSTEnv hence the concern. Once again reading comprehension issue must do better.
There was a problem hiding this comment.
The difference is more subtle than I might have liked, but the alternative would be an initialiser that was way too long!
There was a problem hiding this comment.
initCustomMpiCommQuESTEnv?
|
Looks good not run yet. My only comment is above and raises a wider point where we need to think about how we managed change in the programming interface as we probably want to stabilise as much as possible and only add to existing functionality. |
Agreed, If we're being proper about it, breaking changes to the API require a new major version release, and I don't think we have the time or the inclination currently. |
|
Remaining testing required:
|
iarejula-bsc
left a comment
There was a problem hiding this comment.
Overall, looks good to me, really like the idea! I was wondering if this design would allow running quregs in parallel, each with their own communicator (different envs for each one, maybe?), or if that would be impossible with the current implementation. I think that would be a highly interesting feature. As my team is trying to apply malleability to QuEST, that would be a huge win for us.
A few small comments below, but note this is an untested review.
Co-authored-by: iarejula-bsc <inigo.arejula@bsc.es>
We now check |
|
I note from some testing that the Edit: ye olde print debugging indicates the |
|
Sorry for my own delay! A horrifying week and another on the way, but I have most of today to rip through the outstanding stuff for v4.3 (🤞 🤞 )
I originally misinterpreted you and typed the below response - which I now realise you already understand. Kept it for posterity! I note the autodeployer already relaxes the requirement that all
Ooh that's a super interesting idea; similar to an automatic choosing of a subcommunicator for a
That would be the communicator that QuEST first uses if you do not override it (like right now, but ofc we don't use the world group), which includes all available nodes. Just a way for a user to stop setting their custom comm upon subsequently created
Oh interesting - so we would let users have "full MPI control" at QuESTEnv initialisation, but only limited MPI control of individual
Probably different comm
Ok that's fair enough! Especially if it all ends up in a dedicated MPI-specific header. Totally agreed the more general functionality ( When I get a chance, I will comb through the different diff with some nits 😉 |
Thanks Tyson! Good to know that we already have a concept of incompatible Quregs, so it wouldn't be a totally new concept within the codebase. @iarejula-bsc: Let us know how you would like to be recorded (by name or username) in the contribution file for your contribution here! |
|
Hi, i have been really busy and i didnt take a look over the last discussion, is there anything i should help?
You can use my full name: Íñigo Aréjula Aísa |
|
Would have been handy to know this is ready for review before being merged! Will note some minor comments now we can address in the future. Can we add an example in |
There was a problem hiding this comment.
I will address these when I get a sec, but here's some feedback! (marking my progress with 👀 )
(addressed most in #762)
| int isMultithreaded; | ||
| int isGpuAccelerated; | ||
| int isDistributed; | ||
| bool userOwnsMpi; |
There was a problem hiding this comment.
This should be an int, or the other fields updated to bool, since they are all on equal footing. The existing is* fields are always set to 0 or 1. Possibly there was confusion with the arguments of initCustomQuESTEnv() which accepts similarly named arguments as -1 in order for the auto-deployer to override them - that never reaches the QuESTEnv.
|
|
||
| #if QUEST_COMPILE_MPI && QUEST_COMPILE_SUBCOMM | ||
|
|
||
| #include <stdbool.h> |
There was a problem hiding this comment.
Why is the C bool being imported here? This is an internal C++ file and can use the inbuilt C++ bool
| } | ||
|
|
||
| // initialise QuEST around that communicator | ||
| initCustomMpiQuESTEnv(useDistrib, userOwnsMpi, useGpuAccel, useMultithread); |
There was a problem hiding this comment.
This function (initCustomMpiCommQuESTEnv) doesn't validate the input parameters, and is calling another API function (initCustomMpiQuESTEnv) which does validate. When the latter throws, it will use the latter's validation error message; __func__ will report the latter threw the error, not the former. So the user will see the name of a function they did not call
|
|
||
| #if QUEST_COMPILE_MPI | ||
| MPI_Comm comm_getMpiComm() { | ||
| return mpiCommQuest; |
There was a problem hiding this comment.
This should probably check that mpiCommQuest != MPI_COMM_NULL and otherwise throw an internal error, to protect against an internal caller prematurely accessing it, before it is overwritten from its default
| #if QUEST_COMPILE_MPI | ||
| #include <mpi.h> | ||
|
|
||
| static MPI_Comm mpiCommQuest = MPI_COMM_NULL; |
There was a problem hiding this comment.
This is a global singleton and should use the convention of beginning with global or global_.
Also, the Quest suffix seems redundant given it's a variable private to this internal QuEST function
| void error_commInvalidMpiComm() { | ||
|
|
||
| raiseInternalError("The supplied MPI communicator was MPI_COMM_NULL, or duplication failed."); | ||
| } |
There was a problem hiding this comment.
Duplication failure fine as an internal error, but we can check for MPI_COMM_NULL at user-input validation stage
| void error_commDoubleSetMpiComm() { | ||
|
|
||
| raiseInternalError("An attempt was made to set mpiCommQuest after it had already been set, as indicated by mpiCommQuest != MPI_COMM_NULL."); | ||
| } |
There was a problem hiding this comment.
Ideally caught by user validation
| REQUIRE( (env.isMultithreaded == 0 || env.isMultithreaded == 1) ); | ||
| REQUIRE( (env.isGpuAccelerated == 0 || env.isGpuAccelerated == 1) ); | ||
| REQUIRE( (env.isDistributed == 0 || env.isDistributed == 1) ); | ||
| REQUIRE( (env.userOwnsMpi == 0 || env.userOwnsMpi == 1) ); |
There was a problem hiding this comment.
This is pointless given userOwnsMpi is a bool
There was a problem hiding this comment.
Ah yes, artifact from when it wasn't!
|
|
||
| #include "quest/include/config.h" | ||
|
|
||
| #if QUEST_COMPILE_MPI && QUEST_COMPILE_SUBCOMM |
There was a problem hiding this comment.
I'm not thrilled about guarding the entire header, and the contextualising of exposing subcommunicator as "compiling". Semantics like "expose subcommunicator" might have been better, since all the internal trickery of "don't expose internal subcommunicator-specific functions to other TUs" is brittle when QUEST_COMPILE_MPI already safely guards exposure to MPI types.
This is a half-thought for a future me
| raiseInternalError("A function attempted to communicate via more messages than permitted (since there would be more uniquely-tagged messages than the tag upperbound)."); | ||
| } | ||
|
|
||
| void error_commDoubleSetMpiComm() { |
There was a problem hiding this comment.
This name is hard to parse (double?!), and asymmetric with the existing error_commAlreadyInit. Could be clarified to:
error_commAlreadyHasSetMpiComm`| * Advanced initialiser which lets the user positively declare that they take responsibility for MPI. | ||
| * This means we assume they have called MPI_Init, and that they will call MPI_Finalize. | ||
| */ | ||
| void initCustomMpiQuESTEnv(int useDistrib, bool userOwnsMpi, int useGpuAccel, int useMultithread); |
There was a problem hiding this comment.
Given the user needs to resolve MPI in order to own it, it seems fair to move this into the same "optional advanced-MPI user" module needed by subcommunicator. I think relegating this to an alternate header is better give this is anticipated to be tentatively, and is opaquely named. It can then live in a dedicated doc group, e.g. advanced_env, or similarly
* Fix applyMultiStateControlledSqrtSwap argument list (#738) Remove numControls argument from applyMultiStateControlledSqrtSwap overloaded definition taking std::vector<int> (cherry picked from commit 9c20792) Co-authored-by: D-Exposito <dexposito@cesga.es> * Trotterisation test update (#728) * tests/unit/trotterisation.cpp: updated to use REQUIRE_AGREE and cached statevecs and densmats, and both permutePaulis options * tests/utils/compare.hpp/cpp: added setters for test epsilon * tests/unit/trotterisation.cpp: adjusted test epsilon for quad precision imaginary time evolution tests * tests/unit/trotterisation.cpp: moved unitary time evo test to REQUIRE_AGREE * tests/utils/cache.hpp/cpp: added additional utilities for creating and destroying temp caches (which I guess makes them not caches?) with a set number of qubits * tests/unit/trotterisation.cpp: updated unitary time evo test to test across deployments * tests/unit/trotterisation.cpp: reduced number of qubits and increased number of steps to admit the possibility of testing density matrices too * tests/unit/trotterisation.cpp: added density matrix tests * reduce test precision to lazily pass CPU clang quad-precision * skip Trotter tests in paid CI * changing varname convention * renaming cache funcs --------- Co-authored-by: Oliver Thomson Brown <8394906+otbrown@users.noreply.github.com> Co-authored-by: Tyson Jones <tyson.jones.input@gmail.com> * added Daniel Patino to authorlist * CMake warn when non-release build (#742) --------- Co-authored-by: Oliver Thomson Brown <otbrown@users.noreply.github.com> * Stop Trotter funcs mutating PauliStrSum (#740) Formerly, the Trotter functions (such as applyTrotterizedPauliStrSumGadget()), when passed permutePaulis=true, would randomly permutate the order of the passed PauliStrSum, mutating it and affecting the outputs of subsequent functions like reportPauliStrSum(). The function also contained superfluous memory allocs/copies equal in size to the PauliStrSum. Now, the PauliStrSum is never mutated, and an internally allocated ordering list keeps track of the randomised permutation. We also updated the doc, renamed permutePaulis to permuteTerms, and improved validation. Note that 'permuteTerms' had not yet reached main/release, so these changes do not need to be documented in the v4.3 release notes. * Created custom backend complex types (#729) Created cpu_qcomp and gpu_qcomp (from a shared base_qcomp) to avoid std::complex arithmetic operators in hot loops which caused performance issues. Removed all prior compiler flags and related scaffolding attempting to mitigate the performance issue. Also gave MSVC build the params `/Zc:preprocessor -Xcompiler=/Zc:preprocessor /bigobj` as needed for compilation of the unit tests on my windows machines. * Replace vector<int> with SmallList (a stack array) (#743) This is to circumvent the std::vector performance overheads visible in few-qubit simulation (responsible for a performance regression from v3; see #720), and also so that qubit lists can be passed directly to CUDA kernels without conversion (as explored in #739). * Added few-qubit optimisations (#750) Optimisations include: - Adopted SmallView (const SmallList&) to avoid superfluous SmallList copies - Made internally created matrices static - Change accelerator dynamic function vectors to static arrays - Exit all validators early when validation is disabled Additional cleanup includes: - Tidied accelerator macros (replaced param-specific macros like "numCtrls" and "numTargs" with "param") - Fill ctrlStates vectors with default before localiser - Renamed getBitsFromInteger to setToBitsOfInteger - Adopted const in bitwise.hpp to better express intent Note that the naming of SmallList and SmallView will be subsequently changed to List64 and ConstList64 * Renamed debug API functions to contain "QuEST" (#752) * Renamed environment variables to begin with"QUEST" (#755) * Renamed CMake vars and preprocessors (#756) such that they all begin with QUEST, but some have additional changes * Renamed Small(List|View) to (Const)List64 (#757) * Defer Catch2 test discovery so that we can compile MPI tests on systems which cannot actually run with MPI, because they are missing an MPI or UCX library file, as is witnessed in the CI (when compiling with MPICH). It's generally irksome too to trigger an execution of the test binary (which itself initialises QuEST) during build when on a HPC platform with distinct submit and compute nodes * Enable user to take ownership of MPI (#722) * Added ENABLE_SUBCOMM build option * Moved from MPI_COMM_WORLD to mpiQuestComm * Decided passing *MPI_Comm was probably overly cautious, and updated function name to comm_getMpiComm * environment.cpp: added methods to reset rank and numNodes, and reporting for subcomm compiled * comm_config.hpp/cpp: added comm_setMpiComm * CMakeLists.txt: PUBLIC MPI::MPI_CXX turned out to be unhelpful, even for SubComm, because of course it enforces CXX * Added new custom QuESTEnv initialiser which allow user to positively declare that they take ownership of MPI * validation.cpp: updated comm_end call * comm_config.hpp: added config.h include so COMPILE_MPI is actually defined * subcommunicator.h/cpp: implemented QuESTEnv initialiser with custom MPI_Comm * CMake: added subcommunicator.cpp * comm_config.hpp: added missing config.h include... * comm_config.cpp: explicitly initialise mpiCommQuest to MPI_COMM_NULL, updated setComm for init only workflow * quest.h: added subcommunicator header * CMake: added MPI to application binaries when SUBCOMM is enabled * comm_routines.cpp: post Irecv before Isend which probably won't do anything but it makes MPI library implementers less nervous * tests: added new env test for initCustomMpiQuESTEnv * Added error throws to comm_config to cover new scenarios of badness with user owned MPI * subcommunicator.cpp: updated var names to match QuEST style * tests/unit/initialisations.cpp: slightly modified setQuregAmps test to avoid unexpected test failure due to range checking when compild in Debug configuration * Updated validation in comm_setMpiComm Co-authored-by: iarejula-bsc <inigo.arejula@bsc.es> * userOwnsMpi int->bool * comm_config.cpp: corrected call to MPI_Comm_free * subcommunicator.cpp: userOwnsMpi int->bool * subcommunicator.cpp: added comm_isInit guard around comm_setMpiComm * environment.cpp: USER_OWNS_MPI -> userOwnsMpi * comm_init: fixed case where useDistrib = 0 and userOwnsMpi = true * comm_init: moved (recently) misplaced MPI_Init * AUTHORS.txt: added iarejula-bsc * Added placeholder docstrings to new initialisers * docs/cmake.md: added ENABLE_SUBCOMM to list of QuEST CMake vars * Newly added COMPILE_MPI -> QUEST_COMPILE_MPI * ENABLE_SUBCOMM -> QUEST_ENABLE_SUBCOMM * CMake: corrected OpenMP and subcommunicator pre-processor definitions --------- Co-authored-by: Oliver Thomson Brown <8394906+otbrown@users.noreply.github.com> Co-authored-by: iarejula-bsc <inigo.arejula@bsc.es> * Add flush and sync around prints (#763) to reduce the likelihood of users printing from non-root nodes interrupting QuEST root output. This is not bullet-proof; we sync the active communicator rather than MPI_COMM_WORLD so the user-controlled non-participating processes may still be printing. Furthermore, even if all processes participate, some may have outstanding non-root prints that are not aggregated to the user screen by the time MPI_Barrier finishes. But these syncs greatly reduce the change of corruption, and are effectively free! * Add GPU-aware MPICH detection This enables CRAY MPICH platforms to leverage GPU-awareness, greatly accelerating distributed GPU simulation Co-authored-by: JPRichings <james.richings@ed.ac.uk> * Cleanup custom MPI flow (#762) Important changes: - permit user initialisation of MPI when QuEST is not distributed - changed QuESTEnv fields bool from int (e.g. isMultithreaded) - add user-input validation for custom MPI calls - disambiguated comm_config.cpp concepts of "MPI is initialised" (comm_isMpiInit) from "QuEST communication is active" (comm_isActive) - refactored comm_config.cpp flow, especially related to pre-quest-init flow (during validation) - added Oliver's custom-MPI examples (from #712) - moved new API functions to experimental.h - tweaked reportQuESTEnv output grouping * Added user-control of GPU num threads per block (#736) Added: - QUEST_DEFAULT_NUM_GPU_THREADS_PER_BLOCK CMake option - QUEST_DEFAULT_NUM_GPU_THREADS_PER_BLOCK environment variable - setQuESTNumGpuThreadsPerBlock() API function - getQuESTNumGpuThreadsPerBlock() API function - set_num_gpu_threads examples in examples/extended --------- Co-authored-by: Oliver Thomson Brown <8394906+otbrown@users.noreply.github.com> Co-authored-by: Tyson Jones <tyson.jones.input@gmail.com> * Fix compiler warnings (#770) Beware this included removing the superfluous `numControls` argument from the C++only `std::vector` overload of `applyMultiStateControlledCompMatr2`, which is technically a teeny tiny API break ¯\_(ツ)_/¯ * tests/unit/debug.cpp: updated setQuESTSeeds validation tests to include new validation (#771) Updated number of seeds test to use a valid pointer and added a separate NULL pointer test. * Fix Windows CI test_free.yml: added Release config to ctest commands (#773) --------- Co-authored-by: D-Exposito <dexposito@cesga.es> Co-authored-by: Oliver Thomson Brown <8394906+otbrown@users.noreply.github.com> Co-authored-by: Tyson Jones <tyson.jones.input@gmail.com> Co-authored-by: iarejula-bsc <inigo.arejula@bsc.es> Co-authored-by: JPRichings <james.richings@ed.ac.uk>
Closes #712.
MPI_COMM_WORLDeverywhere. Instead it usesmpiCommQuest(which will very often just be a duplicate ofMPI_COMM_WORLD). This aligns with best practise for library developers, as it helps avoid avoid clashes with user MPI calls.mpiCommQueststuff was broken essentially any QuEST program would also be broken.MPI_COMM_WORLD. This interface is guarded behind compile-time flags as it requires bringing the MPI header into the interface, which means all consumers of libQuEST need to include MPI as well. This interface is not yet documented or tested. I think this is tolerable in the short term as it should only appeal to advanced users.Speaking of tests! Test results on my laptop:
and min_example output:
There is a caveat here: distributed tests on Cirrus keep failing, but this appears to be true of devel as well as this branch, so I'll do some more digging and raise it as a separate issue once I have a better of idea of what is going on.