Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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

Choose a reason for hiding this comment

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

Rewrite as such to make it clearer. This version does not accept the wildcard syntaxes with the features:

Suggested change
_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})$'



_PIPELINE_STAGES = (
Expand Down
96 changes: 53 additions & 43 deletions reframe/core/runtime.py
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}']
Comment on lines +295 to +296
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
valid_matches = ['*', '*:*', sysname, f'{sysname}:*', f'*:{partname}',
f'{part.fullname}']
syspart_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
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this a valid_match. It starts as true and may be turned to false if an exact partition match is requested.

Suggested change
syspart_match = True
valid_match = True

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe adjust the comments too, if needed.

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(('+', '-', '%')):
Copy link
Contributor

@vkarak vkarak Mar 25, 2026

Choose a reason for hiding this comment

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

I think an else here should be enough as the syntax spec is already validated agains the regex:

Suggested change
elif not subspec.startswith(('+', '-', '%')):
else:

# 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
):
Comment on lines +341 to +344
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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):

return True

return False

Expand Down
2 changes: 2 additions & 0 deletions unittests/test_pipeline.py
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']
Comment on lines +362 to +363
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some more tests here:

Suggested change
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 = ['']
Comment on lines 365 to 366
Copy link
Contributor

Choose a reason for hiding this comment

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

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']

Expand Down
Loading