use nodes option and factor out common required config #310
Conversation
smoors
changed the title
eessi/testsuite/common_config.py
Outdated
| partition.update({'environs': ['default']}) | ||
| if partition['scheduler'] in ['slurm', 'squeue']: | ||
| # use --nodes option to ensure the exact number of nodes is requested | ||
| partition['sched_options'] = {'use_nodes_option': True} |
There was a problem hiding this comment.
What if someone already has set other sched_options? This will overwrite that. I think we should just update what's there, no?
eessi/testsuite/common_config.py
Outdated
|
|
||
| for system in site_configuration['systems']: | ||
| for partition in system['partitions']: | ||
| partition.update({'environs': ['default']}) |
There was a problem hiding this comment.
I'm wondering if, in general, we should check if the option is set already - and if so leave it alone (and print a message that we're not changing it, maybe)? I mean this common-config stuff is nice, but if someone has already set it explicitly for their system, they probably want to use that value?
Regardless of whether we want the common config or the existing config to take priority, I don't think we should overwrite it without notification - it may lead to hard-to-debug errors since people think "Huh, but I set environs to XYZ, yet it doesn't respect it?"
There was a problem hiding this comment.
we currently only support the default environ; any other environ will be ignored, so it doesn't make sense to have other environs in the site config
it also doesn't make sense for users to define it in the site config because it's not a site configuration, it's an EESSI test suite configuration.
also, if in future we support other environments, we should add them here in the common config, so i think it's better that the common config takes full control.
but good idea to add a note/warning so the user knows what's going on.
There was a problem hiding this comment.
for the other common config options, those are also required for the test suite to work properly, so they should also be set regardless of what the user has set.
There was a problem hiding this comment.
in aedce55, the PR is updated to always notify when a site config option was changed (but not when it was added).
2 participants
fixes #304