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.

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?



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

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