[feat] Add support for combining system:partition and features/extras in valid_systems#3613
Conversation
…id_systems, as requested in reframe-hpc#2820
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3613 +/- ##
========================================
Coverage 91.37% 91.37%
========================================
Files 62 62
Lines 13484 13484
========================================
Hits 12321 12321
Misses 1163 1163 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I've done some concrete testing to prove this works. Note that I have 4 partitions (rome, genoa, gpu_A100, gpu_H100), of which the first two have the **Case 1: traditional `system:partition` assignment**Output: **Case 2: Combining system:partition with +feat1**Output: **Case 3: Combining system:partition with -feat1**Output: **Case 4: Using only features**This case clearly doesn't make any sense, because it'll create tests where the first element of Output: |
|
Hm, I have to recheck how the current |
|
@casparvl I see the problem you are facing, but I don't agree with the solution conceptually. Allowing a syntax like The problem is more fundamental. It is the fact that This pattern is not unique to |
|
@vkarak Sorry for the slow response, I was on holiday.
Hm, coming from the more traditional syntax before features/extras where a thing, I see your point. But your solution makes the pretty hard assumption that the In this context, having both a I'm not saying there aren't any other options: if we have access to configuration objects in the after-init hook (as discussed in #3323 ), I guess we could take the But: such an approach does feel very clunky, as we're bypassing the whole feature mechanism. Compared to that, I'd find the |
to be blunt, i don't see what's contradictory here. i understand why it looks strange to you, but this PR solves our problem in a relatively simple and generic way. is there any technical reason why this can't be merged? |
|
There's also |
|
Let's step back a bit from the solution and try to understand the problem. Could you please show me a concrete example of these two cases:
It doesn't have to be the full test. I need to see:
|
|
I checked the eessi-testsuite code and I believe I recognized the pattern in the issue description. I guess you want to replace your custom |
|
After quite some thought I think I am now more positive for the The pinning of the system is the only way I can think of currently to properly parameterize over a set of partition parameters. Imagine the following test: class Test(...):
valid_systems = ['+gpu']
gpu = parameter(gpus_by_partition(valid_systems))
config = parameter(['foo', 'bar'])
@run_after('init')
def set_more_constraints(self):
constr = self.gpu[0] # This gives the valid sys:part
if self.config == 'foo':
constr += ' +foo'
self.valid_systems = [constr]This test expresses its initial intent with I am not convinced for the necessity of the other On another note regarding system parameterization, I think the framework should provide a special type of parameter that will do the pinning of the system and environment transparently for such cases and avoid the ugly tuple-based parameterization. Also it should provide an easy way to extend a constraint. Ideally, this should be written as: class Test(...):
valid_systems = ['+gpu']
gpu = system_parameter(gpus_by_partition(valid_systems))
config = parameter(['foo', 'bar'])
@run_after('init')
def set_more_constraints(self):
if self.config == 'foo':
self.valid_systems @= ['+foo'] |
Yes, though
the reason for wanting to replace it is actually because we want to use ReFrame's concept of environment to discover modules across different versions of EESSI. And the available modules are different accross the different EESSI versions.
That's much better phrasing than I used, but that's indeed exactly what we are doing: adding constraint after initialization, but before specialization. In your example, you're basing the extra feature on a parameter value (if a certain parameter is
Exactly. I think that the ability to express something like this (and express it 'late', i.e. right before specialization, so that it can include parameter-based conditions) opens up interesting possibilities for people implementing tests in ReFrame.
I don't personally care too much about the *-based syntaxes myself. Do you care enough that you'd to actively prevent combining
Honestly, I don't think it's worth the extra trouble and code complexity - but let me know if you feel differently. |
vkarak
changed the title
vkarak
left a comment
vkarak
left a comment
There was a problem hiding this comment.
I made some suggestions for improving this PR based on our the discussion. Some are aesthetic. We will also need to update the docs.
|
|
||
| _S = rf'({_NW}(:{_NW})?)' # system/partition | ||
| _VALID_SYS_SYNTAX = rf'^({_S}|{_FKV}(\s+{_FKV})*)$' | ||
| _VALID_SYS_SYNTAX = rf'^(({_FKV}\s+)*{_S}(\s+{_FKV})*|{_FKV}(\s+{_FKV})*)$' |
There was a problem hiding this comment.
Rewrite as such to make it clearer. This version does not accept the wildcard syntaxes with the features:
| _VALID_SYS_SYNTAX = rf'^(({_FKV}\s+)*{_S}(\s+{_FKV})*|{_FKV}(\s+{_FKV})*)$' | |
| _SE = rf'({_N}(:{_N})?)' # system/partition (exact match; no wildcards) | |
| # Valid syntax variants | |
| # | |
| # _SV1: system/partition combinations w/ wildcards | |
| # _SV2: features and extras only | |
| # _SV3: exact system/partition w/ features and extras | |
| _SV1 = rf'{_S}' | |
| _SV2 = rf'{_FKV}(\s+{_FKV})*' | |
| _SV3 = rf'({_FKV}\s+)*{_SE}(\s+{_FKV})*' | |
| _VALID_SYS_SYNTAX = rf'^({_SV1}|{_SV2}|{_SV3})$' |
| elif subspec.startswith('%'): | ||
| key, val = subspec[1:].split('=') | ||
| props[key] = val | ||
| elif not subspec.startswith(('+', '-', '%')): |
There was a problem hiding this comment.
I think an else here should be enough as the syntax spec is already validated agains the regex:
| elif not subspec.startswith(('+', '-', '%')): | |
| else: |
| valid_matches = ['*', '*:*', sysname, f'{sysname}:*', f'*:{partname}', | ||
| f'{part.fullname}'] |
There was a problem hiding this comment.
| valid_matches = ['*', '*:*', sysname, f'{sysname}:*', f'*:{partname}', | |
| f'{part.fullname}'] | |
| syspart_matches = ['*', '*:*', sysname, f'{sysname}:*', f'*:{partname}', | |
| f'{part.fullname}'] |
| plus_feats = [] | ||
| minus_feats = [] | ||
| props = {} | ||
| syspart_match = True |
There was a problem hiding this comment.
I would call this a valid_match. It starts as true and may be turned to false if an exact partition match is requested.
| syspart_match = True | |
| valid_match = True |
There was a problem hiding this comment.
Maybe adjust the comments too, if needed.
| if ( | ||
| have_plus_feats and not have_minus_feats and have_props | ||
| and syspart_match | ||
| ): |
There was a problem hiding this comment.
| if ( | |
| have_plus_feats and not have_minus_feats and have_props | |
| and syspart_match | |
| ): | |
| if (have_plus_feats and | |
| not have_minus_feats and | |
| have_props and | |
| valid_match): |
| hellotest.valid_systems = ['sys:part +x0'] | ||
| hellotest.valid_systems = ['+x0 sys:part'] |
There was a problem hiding this comment.
Add some more tests here:
| hellotest.valid_systems = ['sys:part +x0'] | |
| hellotest.valid_systems = ['+x0 sys:part'] | |
| hellotest.valid_systems = ['sys:part +x0 +y0'] | |
| hellotest.valid_systems = ['sys:part +x0 +y0 %z0=w0'] | |
| hellotest.valid_systems = ['+x0 sys:part'] | |
| hellotest.valid_systems = ['+x0 sys:part +y0 %z0=w0'] |
| with pytest.raises(TypeError): | ||
| hellotest.valid_systems = [''] |
There was a problem hiding this comment.
And also add more tests for invalid syntaxes:
with pytest.raises(TypeError):
hellotest.valid_systems = ['sys:* +foo']
with pytest.raises(TypeError):
hellotest.valid_systems = ['*:part +foo']
with pytest.raises(TypeError):
hellotest.valid_systems = ['*:* +foo']
with pytest.raises(TypeError):
hellotest.valid_systems = ['* +foo']
with pytest.raises(TypeError):
hellotest.valid_systems = ['sys0:part0 sys0:part1 +foo']
with pytest.raises(TypeError):
hellotest.valid_systems = ['sys0:part0 +foo sys0:part1']
with pytest.raises(TypeError):
hellotest.valid_systems = ['+foo sys0:part0 sys0:part1']4 participants
This was requested in #2820 , although that was eventually closed with a suggestion for a workaround.
However, I hit it again when trying to use
reframe.utils.find_modules(substr, ...)in combination with wanting to specify additional features. Example use cases (and there's many more one can come up with):CPUfeature to my CPU partitions (but not to my GPU partitions) and then requesting theCPUfeature from my CPU-only tests.infinibandfeature.In both cases, we want to test all modules with
XorYrespectively that are available on our system, hence we want to use thefind_modulesfunction. However, the first element of the tuple this returns is thesystem:partitionstring for which it found the module matching the requested substring. This is clearly meant to be used invalid_systemsdirectly (as is confirmed by the example in the docs), but for the examples above, you may want to only run on a subset of thesesystem:partitioncombinations (based on the required features. It would be very natural to then do:To make sure that a partition is only valid if it provides the right module and the right feature.
It turned out that the code changes needed are very limited (it seems like a lot because the indentation changed, but the key part is:
elifon the subspec:And adding that
syspart_matchto the return logic:Note that this logic does not assume the
system:partitionto strictly come before or after the features/extras, so I've adapted the valid syntax specification to allow both:If anything, I feel the code has become cleaner by not handling
system:partitionas a special, separate case, but as 'just another item' that may occur as a subspec.