-
Notifications
You must be signed in to change notification settings - Fork 121
[feat] 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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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})*)$' | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| _PIPELINE_STAGES = ( | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||||||||||||||||||
| 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 | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would call this a
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(('+', '-', '%')): | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think an
Suggested change
|
||||||||||||||||||
| # 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| return True | ||||||||||||||||||
|
|
||||||||||||||||||
| return False | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add some more tests here:
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| with pytest.raises(TypeError): | ||||||||||||||
| hellotest.valid_systems = [''] | ||||||||||||||
|
Comment on lines
365
to
366
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'] |
||||||||||||||
|
|
||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.