Add support for combining system:partition and features/extras in valid_systems #3613
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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:
|
Author
|
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: |
Author
|
Hm, I have to recheck how the current |
victorusu
reviewed
Jan 29, 2026
|
|
||
| _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})*)$' |
Contributor
There was a problem hiding this comment.
The PR would benefit from a rewrite of this line.
The system definition should not rely on spaces.
All of these definitions should be valid
1. *
2. daint:gpu
3. daint:mc
4. daint:mc +gpu
5. daint:mc +gpu +uenv
6. daint:mc+gpu
7. daint:mc+gpu +uenv
8. daint:mc +gpu+uenv
9. daint:mc +gpu +uenv+modules
10. daint:mc +gpu+uenv +modules
In the current form only the first four cases are valid.
The fifth case is miss interpreted, where one cannot see the +gpu entry.
And the last five case are not valid systems.
This implies that:
- Tests won't run because of a
space, without any hint. Users may lose a lot of time because of a missingspace. - Potential introduction of silent bugs. The addition of a new feature makes the previous invalid.
- Hinders the ability to write tests with partitions that have multiple features.
We should make this feature more robust to the end user and to the future.
What do you think?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.