only error in GH actions if required_mem_per_node or readonly_files are not defined

[smoors]

by default, only emit a warning.

should make developing a test a bit more user-friendly.

[casparvl]

Won't this conflict with other messages with msg_id='1', such as

test-suite/eessi/testsuite/tests/apps/tensorflow/tensorflow.py

Line 134 in 07a5f8f

log_once(self, warn_msg, msg_id='1', level='warning')

I mean, setting a unique message ID within a test class is doable. But if you set a message ID within EESSI_mixin, it's very hard to guarantee that the same message ID won't be reused in a child-class, no?

[casparvl]

Or is the test.__class__.__name__ here something unique to EESSI_mixin? And then does this print once in total, i.e. even if multiple child classes fail to specify readonly_files?

[smoors]

it's been a while, but if i remember well test.__class__.__name__ is equal to EESSI_mixin here, so no conflicts with child classes

[casparvl]

I am a little uncertain if this PR is worth it - and if it doesn't beat the original purpose of the checks for required_mem_per_node and readonly_files to be defined. The whole purpose of checking was that we guide developers that they have to set these.

Is this really such an issue during test development that you want to skip it by default? I mean: why not just set them? It's very easy to set two defaults, and take it from there, no?

• For required_mem_per_node() I typically just define something when I start developing. Just set the maximum number of memory you have per node on the system on which you are developing as a constant, and you have an initial value.
• For readonly_files I typically set [''] if I don't know the files upfront - but usually you do, because you've added them explicitely to the src directory...

Note that for readonly_files I'm also adding a all_readonly_files option in #303 since I hated having to specify every individual file. Typically, I think that's a decent default option for almost any test, to have all_readonly_files=True. I think having files that are not read-only will be the exception.

For the required_mem_per_node, I can understand a bit better why you want to avoid the hard error: you need to at least run it once with report_memory_usage=True in order to get the correct numbers to set a sensible required_mem_per_node. And one could argue that if I've already put in a placeholder, I'm no longer 'forced' to update this. I guess I would be fine with a knob that would turn it into a warning, but I'd make 'error' be the default.

I.e. we could update the docs to instruct that during your first run you might set --setvar required_mem_per_node_undefined_policy=warning --setvar measure_memory_usage = True. Then, once you've figured out the memory requirement (and put it in), you just ommit any command line overrides with --setvar, and you're done. Note that we could even print this a hint in the current error message ("If you are still developing this test and want to suppress this error temporarily, use --setvar required_mem_per_node_undefined_policy=warning").

The advantage of this approach that anyone who reviews that PR will get the error policy by default, and this provides us with a hard check that the contributor of the test did, in fact, implement a correct required_mem_per_node function.

By the way, regarding the readonly_files policies: although I personally don't see the usage (I think setting all_readonly_files = True as starting point and seeing how far that gets you is a very easy, and 95% of the time correct starting point), if you're very eager to have a knob that you can turn, I'm not fundamentally against that. As long as the default is still 'error'. So I guess that's up to you, if you're still very eager (even considering the all_readonly_files attribute that's about to be introduced)...

[smoors]

just to clarify: this PR is not about fixing an issue, it’s quality-of-life improvement. i’m definitely OK with closing it if you don’t agree.

since you made quite an effort to write a review, i’ll write my response for the sake of the argument :)

The whole purpose of checking was that we guide developers that they have to set these.

we are still checking that developers have set it, but only later, when they open a PR. this is useful, not only when developing, but also if you don’t care about merging upstream and if are not interested in running the test on low-memory systems.

you could compare this with checksums in easyconfigs. you're not required to add them (e.g. because you already downloaded the sources and are confident that they are intact), but if you open a PR upstream you have to add them.

why not just set them? It’s very easy to set two defaults, and take it from there, no?

imho, setting random defaults is as bad as not setting anything, because the defaults may be wrong and it's easy to forget to update them later on to the correct value.

For required_mem_per_node() I typically just define something when I start developing. Just set the maximum number of memory you have per node on the system on which you are developing as a constant, and you have an initial value.

setting the maximum memory requires looking up this value in the site config for the partition you want to run it on, and you still have to change this afterward, so that’s 2 steps instead of 1, and again step 2 is easy to forget.

For readonly_files I typically set [’’] if I don’t know the files upfront - but usually you do, because you’ve added them explicitely to the src directory...

here you may also want to change this afterward, and it’s again easy to forget.

The advantage of this approach that anyone who reviews that PR will get the error policy by default, and this provides us with a hard check that the contributor of the test did, in fact, implement a correct required_mem_per_node function.

the reviewer does not need to have the error policy because the CI check already has it, and it will tell you if the required_mem_per_node was implemented correctly.

setting all_readonly_files = True

that's a good addition, and will definitely help.

As long as the default is still 'error'.

if "error" must stay the default, then i don’t see much value in this PR, because changing the default is again extra work for the test developer, which is exactly what this PR is meant to reduce.

so i leave it up to you to decide what to do with this PR.. :)