Fix for quest.h generation

[otbrown]

Hi Tyson,

Hope the unitary hack is going well!

I've finally updated to 4.1 locally and spotted a subtle bug with Luc's custom compile_option function in CMake -- the variables set within it were dutifully thrown away at return. The fix is trivial, just needed to added PARENT_SCOPE to the set command.

As that function also independently set target_compile_definitions the library itself is still compiled correctly, this bug would only impact downstream projects (like my CQ!).

Cheers,
Oliver

[otbrown]

Hmm I'm now getting hit by a bunch of multiple definition errors when attempting to compile CQ-SimBE as a consequence of trying to include quest.h in a header file, fair enough. If I don't include quest.h though I fail a modes.h check.

All of which is to say I think I'm now an advocate of separatiing the COMPILE_XXX definitions into a quest_config.h!

[TysonRayJones]

Drat! It's true that those preprocessors aren't needed by the user source; they only affect backend compilation. They need only "survive" past compilation for users who wish to know the compiled backends during compilation of their own source code. So I'm with you about moving them to a bespoke, optional header.

Then quest.h.in would return to quest.h, and we could adapt modes.h from

// ensure defined
#ifndef COMPILE_MPI
    #error "Compiler must define COMPILE_MPI"
#endif

// ensure valid
#if ! (COMPILE_MPI == 0 || COMPILE_MPI == 1)
    #error "Macro COMPILE_MPI must have value 0 or 1"
#endif

to

// permit undefined else ensure valid
#if ! (COMPILE_MPI == 0 || COMPILE_MPI == 1)
    #error "Macro COMPILE_MPI must have value 0 or 1, or else be undefined"
#endif

// (above exploits undefined preprocessor == 0 in C/C++ standards)

and rename it to preprocessors.h or integrity.h - or something to indicate it merely validates optionally-defined preprocessors.

The new quest_config.h you propose would then contain Luc's cmake-fu, and need only ever be included by "superusers" who demand compile-time knowledge of the compiled backends. It can itself be defensive and include guards to ensure COMPILE_XXX are not defined before its inclusion.

So all agreed! Alas I'm insufficiently knowledgeable in CMake configs to make quest_config.h.in properly, but happy to help out alongside e.g. yourself or @lucjaulmes 🙏

[otbrown]

I think the changes needed are pretty minor, so will try them out over the next few days and hit you with the PR! If I get stuck I'll definitely be asking Luc ;)

[otbrown]

Okay! We no longer generate quest.h, but instead generate quest/include/config.h, for inclusion if needed by problem advanced users like me ;)

[TysonRayJones]

Wew brilliant! 🎉

Enabling non-cmake compilation

Can I throw a minor spanner in the works? 😅 It'd be neat to make extricable the build from cmake so that manual compilation (like through this bash script - though I must update it) remains possible. It's inessential but super useful for debugging compilation.

This is currently only prevented by quest.h containing:

#include "quest/include/config.h"

since config.h does not exist until CMake processes config.h.in. Can we adapt it to avoid seeking config.h when CMake isn't used?

Some quick ideas:

1. guard inclusion of config.h from within quest.h by some preprocessor being defined.
   Then users of quest.h never see COMPILE_XXX unless they define e.g. DEFINE_CONFIG (and they compiled or installed with cmake, of course) before inclusion. Would not affect cmake config generation at all.

   cons: some of the API (arguably unimportant compile-time stuff) is then "hidden by default"

2. guard inclusion of config.h from within quest.h by some preprocessor being not defined. Then "manual compilers" (pejorative) must define e.g. DO_NOT_DEFINE_CONFIG to successfully compile.

   cons: manual compilers must define that macro to compile. This is a bit of a tedium in the intended debugging settings, complicating the compiler arguments which are the very things being scrutinised.

3. remove config.h from quest.h and require users directly include it (renamed to e.g. quest_config.h).

   cons: it's a shame to need multiple includes - and does having such muddy the installation process?

4. rename config.h.in to config.h, guard against the #cmakedefine macros using a cmake-defined preprocessor, configure cmake to process config.h into config_out.h which is included at the bottom of config.h. So config.h prior exists and is always included, and will itself include a secondary cmake-generated file with the permanent macros.

   cons: seems hacky and nonstandard

Do you have any thoughts on these? I lean personally toward 1. with the justification that COMPILE_XXX are outside the main API; they are compile-time and ergo "special", so warrant needing to use a link-time macro to observe.

Other preprocessors

The DEFAULT_VALIDATION_EPSILON macro in precision.h is also advertised as re-configurable at compile-time. Since it is not included in the cmake config, it is possible for users to override it to different values when installing then when including QuEST. Alas changing DEFAULT_VALIDATION_EPSILON from the value used during installation does nothing. Worse, users might override it at installation but then not override it during inclusion (which is natural) but then consult its value which will be wrong.

This does not seem a huge problem: it's "too late" for users to adjust the default epsilon, and they'll instead have to use runtime control. This might be better than caching DEFAULT_VALIDATION_EPSILON by moving it out of precision.h (where it needs to consult FLOAT_PRECISION) into config.h.in. It does however necessitate updating the doc...

QuEST/quest/include/precision.h

Lines 123 to 131 in f3baf34

	/*
	* RE-CONFIGURABLE DEFAULT VALIDATION PRECISION
	*
	* which is compile-time overridable by pre-defining DEFAULT_VALIDATION_EPSILON (e.g.
	* in user code before importing QuEST, or passed as a preprocessor constant by the
	* compiler using argument -D), and runtime overridable using setValidationEpsilon()
	*/
	#ifndef DEFAULT_VALIDATION_EPSILON

to e.g.

/*
 * RE-CONFIGURABLE DEFAULT VALIDATION PRECISION
 *
 * which is overridable when compiling the QuEST source by defining 
 * DEFAULT_VALIDATION_EPSILON. Beware that updating this macro
 * after compilation (e.g. after installing QuEST) has no effect, though
 * epsilon can be overridden at runtime using setValidationEpsilon()
 */

What do you think about this? Is it a pitfall? Will users deliberately exposing the config macros (as per above) be tricked by DEFAULT_VALIDATION_EPSILON (provided by a different header) being distinct from its install-time value which is actually being used by the source?

[otbrown]

RE non-cmake compilation:

So actually, with the removal of the modes header #ifndef COMPILE_XXXX guards, the only thing that doesn't compile if you just remove #include "quest/include/config.h" is one line in the tests main where it checks if cuQuantum is compiled. In fact, even my downstream project doesn't need those compiler definitions now that guarding is removed, so I vote for option 3 -- remove it from quest.h. We should still generate it, as it's nice to have a record of what was compiled, but that's now its main purpose! Users who are treating QuEST strictly as a library no longer need to know or care about these defines, which feels like a win all round.

I would of course also update QuESTEnv to include int isCuQuantum or equivalent to remove the problem line in tests/main.cpp too.

RE DEFAULT_VALIDATION_EPSILON:

To be honest I suspect a user messing with this really needs to know what they're doing, so I'm not too upset about it being fiddly to do :) Here I vote for just updating the doc as you suggest to make it clear it's a property of the QuEST library, not of the application code they build against that library.

[otbrown]

I can see an argument here for expanding config.h to include the default validation epsilon set at compile time, for similar record-keeping/occasional-onward-propagation purposes!

[TysonRayJones]

COMPILE_CUQUANTUM

Aha I'd forgotten about that reference in the tests - I'll bind isCuQuantumEnabled to QuESTEnv as suggested 🙏 Then agreed config.h (or renamed to quest_config.h for standalone inclusion) can be removed from quest.h.

DEFAULT_VALIDATION_EPSILON

You've convinced me that quest_config.h should include DEFAULT_VALIDATION_EPSILON. Will the below "default logic" have to be moved out of the headers and into the CMake build?

QuEST/quest/include/precision.h

Lines 133 to 140 in 4d44ec8

	#if FLOAT_PRECISION == 1
	#define DEFAULT_VALIDATION_EPSILON 1E-5
	#elif FLOAT_PRECISION == 2
	#define DEFAULT_VALIDATION_EPSILON 1E-12
	#elif FLOAT_PRECISION == 4
	#define DEFAULT_VALIDATION_EPSILON 1E-13

PERMIT_NODES_TO_SHARE_GPU

TLDR I will change PERMIT_NODES_TO_SHARE_GPU to an environment variable unless objected to! Reasoning below (in context) for posterity.

The remaining preprocessors which can differ between compilation and include are:

QuEST/quest/include/modes.h

Lines 78 to 88 in 4d44ec8

	#ifndef PERMIT_NODES_TO_SHARE_GPU
	#define PERMIT_NODES_TO_SHARE_GPU 0
	#endif
	#ifndef INCLUDE_DEPRECATED_FUNCTIONS
	#define INCLUDE_DEPRECATED_FUNCTIONS 0
	#endif
	#ifndef DISABLE_DEPRECATION_WARNINGS
	#define DISABLE_DEPRECATION_WARNINGS 0
	#endif

The latter two are unimportant and should be link-time overridable since they merely control user-source-compile-time warnings. But PERMIT_NODES_TO_SHARE_GPU is stickier - it appears it two sources:

QuEST/quest/src/api/environment.cpp

Lines 105 to 111 in 4d44ec8

	// each MPI process must use a unique GPU. This is critical when
	// initializing cuQuantum, so we don't re-init cuStateVec on any
	// paticular GPU (causing runtime error), but still ensures we
	// keep good performance in our custom backend GPU code; there is
	// no reason to use multi-nodes-per-GPU except for dev/debugging.
	if (useGpuAccel && useDistrib && ! PERMIT_NODES_TO_SHARE_GPU)
	validate_newEnvNodesEachHaveUniqueGpu(caller);

QuEST/quest/src/core/validation.cpp

Lines 1361 to 1370 in 4d44ec8

	void validate_newEnvNodesEachHaveUniqueGpu(const char* caller) {
	// this validation can be disabled for debugging/dev purposes
	// (caller should explicitly check this preprocessor too for clarity)
	if (PERMIT_NODES_TO_SHARE_GPU)
	return;
	bool uniqueGpus = ! gpu_areAnyNodesBoundToSameGpu();
	assertAllNodesAgreeThat(uniqueGpus, report::MULTIPLE_NODES_BOUND_TO_SAME_GPU, caller);
	}

(Its second appearance is totally redundant; that must be that 3am flavour of "defensive design" woops 😅 )

The purpose of PERMIT_NODES_TO_SHARE_GPU is just to disable validation which prevent users from mistakingly over-subscribing GPUs and experiencing unexpected performance degradation. It's useful to disable this validation when unit-testing MPI+GPU hybridisation on a single GPU.

If it were to remain a compile-time flag, I suppose it should also be recorded by quest_config.h. BUT there's really no need for this to be fixed at compile-time - it seems a gratuitous tedium to have to recompile/re-install QuEST just to change it! Ideally one should wish to easily change it between launches of a QuEST executable.

It's a little clumsy to be specified at runtime however, since that validation occurs during initQuESTEnv() before which no user-configuration can be done. It also seems gross to add it as a necessary parameter to initCustomQuESTEnv() since it "feels" like a meta setting that should never need to be referenced except when being disabled.

So, it seems prudent to change it to an environment-variable. Would this cause any issues? Would that jeopardise QuEST's ability to run on extremely stripped-down systems like microcontrollers?? 😅

Test macros

Is it okay to neglect the test-specific macros? Are the tests even included in the installation?

QuEST/tests/utils/macros.hpp

Lines 30 to 48 in 4d44ec8

	// 0 = perform all, and a sensible value to accelerate tests is 50
	#ifndef TEST_MAX_NUM_QUBIT_PERMUTATIONS
	#define TEST_MAX_NUM_QUBIT_PERMUTATIONS 0
	#endif
	// 0 = perform all (very slow), while 4 limits to superops = 8-qubit matrices
	#ifndef TEST_MAX_NUM_SUPEROP_TARGETS
	#define TEST_MAX_NUM_SUPEROP_TARGETS 4
	#endif
	// 0 = use all available deployments at once, 1 = try all combinations in-turn
	#ifndef TEST_ALL_DEPLOYMENTS
	#define TEST_ALL_DEPLOYMENTS 1
	#endif
	// number of times to repeat each "[mixed]" test (minimum 1)
	#ifndef TEST_NUM_MIXED_DEPLOYMENT_REPETITIONS
	#define TEST_NUM_MIXED_DEPLOYMENT_REPETITIONS 10
	#endif

[TysonRayJones]

I've had the 🚿 epiphany 🚿 that the discussed overridable preprocessors fall into three categories: those which must be known at...

1. source-compile-time (e.g. COMPILE_XXX)
2. user-source-link-time (e.g. INCLUDE_DEPRECATED_FUNCTIONS)
3. executable launch, before initQuESTEnv() (e.g. PERMIT_NODES_TO_SHARE_GPU)

All 1. must be saved by cmake, all 2. must never be saved, and 3. should never have been preprocessors! Instead, all 3. should be environment variables; they offer no benefit to being decided at source-compile-time which instead reduces flexibility. I propose changing all below macros to environment variables:
(source)

• PERMIT_NODES_TO_SHARE_GPU
• DEFAULT_VALIDATION_EPSILON

(tests)

• TEST_MAX_NUM_QUBIT_PERMUTATIONS
• TEST_MAX_NUM_SUPEROP_TARGETS
• TEST_ALL_DEPLOYMENTS
• TEST_NUM_MIXED_DEPLOYMENT_REPETITIONS

The latter four are a little irritating to refactor since they are accessed outside the source so shouldn't use the internal convenience functions for obtaining env-vars. This puts 'don't repeat yourself' in tension with 'separation of concerns', but I'll find some compromising design.

I'll ergo not merge #650 and instead extend it in a new PR to properly handle environment variables. That should resolve everything; there will be no remaining preprocessors needing saving by cmake, no ineffectual-overridable macros, quest_config.h can be strictly "include as needed" (necessitating cmake compilation), yet the source can remain manually compilable. 👨‍🍳 💋

[otbrown]

Sounds great to me!

(Apologies for the delay, was travelling now attending a conference!)

[TysonRayJones]

Wew brilliant! No problem about the micro-delay - I'm supposed to be on holiday but I have norovirus 😭

I'll merge this PR, commit the env-vars, then will commit renaming of quest_config.h to quest_config.h.in and add some file doc.

One final thing regarding these lines:

QuEST/quest/include/config.h.in

Lines 5 to 9 in 1f1e702

	#if !defined(FLOAT_PRECISION)\
	|| !defined(COMPILE_MPI)\
	|| !defined(COMPILE_OPENMP)\
	|| !defined(COMPILE_CUDA)\
	|| !defined(COMPILE_CUQUANTUM)

Is it correct that this permits gratuitous specification of COMPILE_XXX so that e.g. compiling without installing will work? Is it plausible a user could install QuEST but later erroneously believe they can change the available hardware backends using those preprocessors (and also be including quest_config.h for some reason)? That wouldn't cause any errors, but may cause user astonishment - though they can runtime confirm which deployments are actually compiled (using reportQuESTEnv()).

A user optionally including quest_config.h is seeking to obtain COMPILE_XXX rather than override, so it seems sensible to actually throw a compile-time error if they are gratuitously specifying those preprocessors. I can chuck in the error checks in the aforementioned commit 🙏

[otbrown]

Ooft, really sorry to hear about the norovirus 😬 that's a bad time even if you weren't meant to be on holiday...

Exactly so, this tells the user what was compiled into the QuEST library -- redefining elsewhere will, at best, do nothing. So not a bad idea to warn the user they're being silly. My original logic there was just that they all needed to be defined in the QuEST build, so if any was undefined they should all be defined.

Final question, should we rename them to QUEST_COMPILE_XXXX in case downstream codes have similar ideas around compile time configuration? It seems fairly unlikely to clash, especially now that it's not leaking onward at all, but still...

[TysonRayJones]

Thanks very much! It's pretty grim, though it's making me appreciate all the good work my bowels ordinarily perform.

Gotcha! A QUEST_ prefix seems sensible and standard - I suppose all preprocessors (and maybe even environment variables?!) should be prefixed. Does changing preprocessors constitute an API break needing a major version update? 😓

[otbrown]

I would say no, as user code stays the same! (Unless you were doing something really weird). It only maybe matters to whoever builds the library.

Plus most people will set these preprocessor directives through the magic of CMake, and that interface remains unchanged 😁

[lucjaulmes]

Sorry this is a bit verbose and I’m joining a bit late. But everything seems sorted now, no? 😅

[TysonRayJones]

Ok perf! Let's rename the vars in another PR (after I merge the env-var one) to make that change more explicit. Sorry for the delay in merging this!