Skip to content

Add support for combining system:partition and features/extras in valid_systems #3613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
casparvl wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add support for combining system:partition and features/extras in valid_systems #3613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7092fb9
Add support for combining system:partition and features/extras in val…
Jan 21, 2026
89259dd
See if this fixes the failing unit tests
Jan 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion reframe/core/pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def launch_command(self, stagedir):
_VALID_ENV_SYNTAX = rf'^({_NW}|{_FKV}(\s+{_FKV})*)$'

_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})*)$'
Copy link
Contributor

@victorusu victorusu Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Tests won't run because of a space, without any hint. Users may lose a lot of time because of a missing space.
  2. Potential introduction of silent bugs. The addition of a new feature makes the previous invalid.
  3. 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?

Copy link
Author

@casparvl casparvl Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The system definition should not rely on spaces.

Just to be clear: unless I'm mistaken, the current version of ReFrame also requires spaces between features, right?

All of these definitions should be valid

And same here: definitions 6-10 are not valid in the current reframe either?

Don't get me wrong: I'm not against allowing for non-space separated features. But I'm also not sure it should be handled in this PR: allowing non-space seperated features is orthogonal to being able to combine the sysname:partname syntax with the +feat syntax. And I wouldn't want this PR to be delayed because of discussions on whether or not non-space separated features are desirable.

The fifth case is miss interpreted, where one cannot see the +gpu entry.

That's strange, so you're saying that if you define:

daint:mc +gpu +uenv

You're effectively getting

daint:mc +uenv

?

I haven't observed that, but I might have not tested this particular case. I'll try it out...

Copy link
Author

@casparvl casparvl Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot reproduce this. I have two partitions, A and B. A has feature foo and B has features foo and bar. If I create a test and set

self.valid_systems = ['sysname:* +bar +foo'

your issue was that bar would be ignored. However, I tested this, and it (correctly) only schedules a test for partition B, which offers bar.



_PIPELINE_STAGES = (
Expand Down
96 changes: 53 additions & 43 deletions reframe/core/runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,50 +289,60 @@ def is_env_loaded(environ):


def _is_valid_part(part, valid_systems):
for spec in valid_systems:
if spec[0] not in ('+', '-', '%'):
# This is the standard case
sysname, partname = part.fullname.split(':')
valid_matches = ['*', '*:*', sysname, f'{sysname}:*',
f'*:{partname}', f'{part.fullname}']
if spec in valid_matches:
return True
else:
plus_feats = []
minus_feats = []
props = {}
for subspec in spec.split(' '):
if subspec.startswith('+'):
plus_feats.append(subspec[1:])
elif subspec.startswith('-'):
minus_feats.append(subspec[1:])
elif subspec.startswith('%'):
key, val = subspec[1:].split('=')
props[key] = val

have_plus_feats = all(
(ft in part.features or
ft in part.resources or ft in part.extras)
for ft in plus_feats
)
have_minus_feats = any(
(ft in part.features or
ft in part.resources or ft in part.extras)
for ft in minus_feats
)
try:
have_props = True
for k, v in props.items():
extra_value = part.extras[k]
extra_type = type(extra_value)
if extra_value != extra_type(v):
have_props = False
break
except (KeyError, ValueError):
have_props = False
# Get sysname and partname for the partition being checked and construct
# all valid_matches for the partition being checked
sysname, partname = part.fullname.split(':')
valid_matches = ['*', '*:*', sysname, f'{sysname}:*', f'*:{partname}',
f'{part.fullname}']

if have_plus_feats and not have_minus_feats and have_props:
return True
# If any of the specs in valid_systems matches, this is a valid partition
for spec in valid_systems:
plus_feats = []
minus_feats = []
props = {}
syspart_match = True
for subspec in spec.split(' '):
if subspec.startswith('+'):
plus_feats.append(subspec[1:])
elif subspec.startswith('-'):
minus_feats.append(subspec[1:])
elif subspec.startswith('%'):
key, val = subspec[1:].split('=')
props[key] = val
elif not subspec.startswith(('+', '-', '%')):
# If there is a system:partition specified, make sure it
# matches one of the items in valid_matches
syspart_match = True if subspec in valid_matches else False

have_plus_feats = all(
(ft in part.features or
ft in part.resources or ft in part.extras)
for ft in plus_feats
)
have_minus_feats = any(
(ft in part.features or
ft in part.resources or ft in part.extras)
for ft in minus_feats
)
try:
have_props = True
for k, v in props.items():
extra_value = part.extras[k]
extra_type = type(extra_value)
if extra_value != extra_type(v):
have_props = False
break
except (KeyError, ValueError):
have_props = False

# If the partition has all the plus features, none of the minus
# all of the properties and the system:partition spec (if any)
# matched, this partition is valid
if (
have_plus_feats and not have_minus_feats and have_props
and syspart_match
):
return True

return False

Expand Down
2 changes: 2 additions & 0 deletions unittests/test_pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,8 @@ def test_valid_systems_syntax(hellotest):
hellotest.valid_systems = ['+x0 -y0 %z0=w0']
hellotest.valid_systems = ['-y0 +x0 %z0=w0']
hellotest.valid_systems = ['%z0=w0 +x0 -y0']
hellotest.valid_systems = ['sys:part +x0']
hellotest.valid_systems = ['+x0 sys:part']

with pytest.raises(TypeError):
hellotest.valid_systems = ['']
Expand Down
Loading