From 48cb7aa608a7a1ef5304fb510361b70832410f7a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 12 Nov 2024 17:25:34 +0100 Subject: [PATCH 1/6] Allow partition/environment extras to be used also as feature constraints --- docs/config_reference.rst | 4 +++- reframe/core/pipeline.py | 11 +++++++++-- reframe/core/runtime.py | 11 +++++++---- unittests/test_pipeline.py | 39 ++++++++++++++++++++++++++++++++------ 4 files changed, 52 insertions(+), 13 deletions(-) diff --git a/docs/config_reference.rst b/docs/config_reference.rst index 04d6959d60..cd7d8e510a 100644 --- a/docs/config_reference.rst +++ b/docs/config_reference.rst @@ -601,7 +601,7 @@ System Partition Configuration :required: No :default: ``[]`` - A list of job scheduler `resource specification <#custom-job-scheduler-resources>`__ objects. + A list of job scheduler :ref:`resource specification ` objects. .. py:attribute:: systems.partitions.processor @@ -747,6 +747,8 @@ ReFrame can launch containerized applications, but you need to configure properl If specified in conjunction with :attr:`~systems.partitions.container_platforms.env_vars`, it will be ignored. +.. _scheduler-resources: + Custom Job Scheduler Resources ============================== diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 8c3ab5e46c..5663f6e111 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -261,11 +261,18 @@ def pipeline_hooks(cls): #: of the :attr:`valid_systems` list, in which case an AND operation on #: these constraints is implied. For example, the test defining the #: following will be valid for all systems that have define both ``feat1`` - #: and ``feat2`` and set ``foo=1`` + #: and ``feat2`` and set ``foo=1``: #: #: .. code-block:: python #: - #: valid_systems = ['+feat1 +feat2 %foo=1'] + #: valid_systems = [r'+feat1 +feat2 %foo=1'] + #: + #: Any partition/environment extra or + #: :ref:`partition resource ` can be specified as a + #: feature constraint without having to explicitly state this in the + #: partition's/environment's feature list. For example, if ``key1`` is part + #: of the partition/environment extras list, then ``+key1`` will select + #: that partition or environment. #: #: For key/value pairs comparisons, ReFrame will automatically convert the #: value in the key/value spec to the type of the value of the diff --git a/reframe/core/runtime.py b/reframe/core/runtime.py index 24d754b991..e3fbce980f 100644 --- a/reframe/core/runtime.py +++ b/reframe/core/runtime.py @@ -311,11 +311,13 @@ def _is_valid_part(part, valid_systems): props[key] = val have_plus_feats = all( - ft in part.features or ft in part.resources + (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 + (ft in part.features or + ft in part.resources or ft in part.extras) for ft in minus_feats ) try: @@ -357,8 +359,9 @@ def _is_valid_env(env, valid_prog_environs): key, val = subspec[1:].split('=') props[key] = val - have_plus_feats = all(ft in env.features for ft in plus_feats) - have_minus_feats = any(ft in env.features + have_plus_feats = all(ft in env.features or ft in env.extras + for ft in plus_feats) + have_minus_feats = any(ft in env.features or ft in env.extras for ft in minus_feats) try: have_props = True diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index e5010e255c..6807088b91 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -556,7 +556,7 @@ def _assert_supported(valid_systems, valid_prog_environs, # Check AND in features and extras _assert_supported( - valid_systems=['+cuda +mpi %gpu_arch=v100'], + valid_systems=[r'+cuda +mpi %gpu_arch=v100'], valid_prog_environs=['*'], expected={} ) @@ -566,9 +566,18 @@ def _assert_supported(valid_systems, valid_prog_environs, expected={} ) - # Check OR in features ad extras + # Check OR in features and extras _assert_supported( - valid_systems=['+cuda +mpi', '%gpu_arch=v100'], + valid_systems=['+cuda +mpi', r'%gpu_arch=v100'], + valid_prog_environs=['*'], + expected={ + 'testsys:gpu': ['PrgEnv-gnu', 'builtin'] + } + ) + + # Check that extra keys can used as features + _assert_supported( + valid_systems=['+cuda +mpi', '+gpu_arch'], valid_prog_environs=['*'], expected={ 'testsys:gpu': ['PrgEnv-gnu', 'builtin'] @@ -640,7 +649,7 @@ def _assert_supported(valid_systems, valid_prog_environs, ) _assert_supported( valid_systems=['*'], - valid_prog_environs=['%bar=x'], + valid_prog_environs=[r'%bar=x'], expected={ 'testsys:gpu': [], 'testsys:login': ['PrgEnv-gnu'] @@ -648,7 +657,7 @@ def _assert_supported(valid_systems, valid_prog_environs, ) _assert_supported( valid_systems=['*'], - valid_prog_environs=['%foo=2'], + valid_prog_environs=[r'%foo=2'], expected={ 'testsys:gpu': ['PrgEnv-gnu'], 'testsys:login': [] @@ -656,7 +665,7 @@ def _assert_supported(valid_systems, valid_prog_environs, ) _assert_supported( valid_systems=['*'], - valid_prog_environs=['%foo=bar'], + valid_prog_environs=[r'%foo=bar'], expected={ 'testsys:gpu': [], 'testsys:login': [] @@ -671,6 +680,24 @@ def _assert_supported(valid_systems, valid_prog_environs, } ) + # Check that extra keys can used as features + _assert_supported( + valid_systems=['*'], + valid_prog_environs=['+foo +bar'], + expected={ + 'testsys:gpu': ['PrgEnv-gnu'], + 'testsys:login': ['PrgEnv-gnu'] + } + ) + _assert_supported( + valid_systems=['*'], + valid_prog_environs=['+foo -bar'], + expected={ + 'testsys:gpu': [], + 'testsys:login': [] + } + ) + # Check valid_systems / valid_prog_environs combinations _assert_supported( valid_systems=['testsys:login'], From 4052242102b855f21df8ebea59d2c6baba064b09 Mon Sep 17 00:00:00 2001 From: Blanca Fuentes Date: Fri, 25 Oct 2024 15:49:18 +0000 Subject: [PATCH 2/6] Add undefined parameters in error message --- reframe/core/decorators.py | 22 +++++++++++++++++----- reframe/core/fixtures.py | 8 +++++++- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index b8fbc7e22b..1a7d02b7cf 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -8,7 +8,7 @@ # __all__ = ['simple_test'] - +_TREAT_WARNINGS_AS_ERRORS = False import inspect import sys @@ -104,7 +104,7 @@ def instantiate_all(self, reset_sysenv=0, external_vars=None): kwargs['reset_sysenv'] = reset_sysenv leaf_tests.append(test(*args, **kwargs)) except SkipTestError as e: - getlogger().verbose( + getlogger().warning( f'skipping test {test.__qualname__!r}: {e}' ) except Exception: @@ -170,11 +170,23 @@ def _validate_test(cls): 'subclass of RegressionTest') if (cls.is_abstract()): + params = cls.param_space.params + undefined_params = [] + for param in params: + if params[param].is_abstract(): + undefined_params.append(param) + # Raise detailed warning + if _TREAT_WARNINGS_AS_ERRORS: + raise ValueError( + f'skipping test {cls.__qualname__!r}: ' + + f'test has the following undefined parameters: ' + + ', '.join(undefined_params) + ) getlogger().warning( - f'skipping test {cls.__qualname__!r}: ' - f'test has one or more undefined parameters' + f'skipping test {cls.__qualname__!r}: ' + + f'test has the following undefined parameters: ' + + ', '.join(undefined_params) ) - return False conditions = [VersionValidator(v) for v in cls._rfm_required_version] if (cls._rfm_required_version and diff --git a/reframe/core/fixtures.py b/reframe/core/fixtures.py index e285467974..21f71b7585 100644 --- a/reframe/core/fixtures.py +++ b/reframe/core/fixtures.py @@ -775,8 +775,14 @@ def __init__(self, cls, *, scope='test', action='fork', variants='all', # Check that the fixture class is not an abstract test. if cls.is_abstract(): + params = cls.param_space.params + undefined_params = [] + for param in params: + if params[param].is_abstract(): + undefined_params.append(param) raise ValueError( - f'class {cls.__qualname__!r} has undefined parameters' + f'class {cls.__qualname__!r} has undefined parameters: ' + + ', '.join(undefined_params) ) # Validate the scope From 2e3e0366b88f4c3965ed7205111d4a6ec3b8848a Mon Sep 17 00:00:00 2001 From: Blanca Fuentes Date: Fri, 25 Oct 2024 15:50:37 +0000 Subject: [PATCH 3/6] Add unit tests for error/warning message --- unittests/test_fixtures.py | 4 +++- unittests/test_parameters.py | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/unittests/test_fixtures.py b/unittests/test_fixtures.py index 2fe388a8f9..72c99e0144 100644 --- a/unittests/test_fixtures.py +++ b/unittests/test_fixtures.py @@ -71,10 +71,12 @@ def test_abstract_fixture(): class Foo(rfm.RegressionTest): p = parameter() - with pytest.raises(ValueError): + with pytest.raises(ValueError) as exc_info: class MyTest(rfm.RegressionMixin): f = fixture(Foo) + assert "p" in str(exc_info.value) + def test_fixture_variants(): '''Fixtures must have at least one valid variant.''' diff --git a/unittests/test_parameters.py b/unittests/test_parameters.py index 118bba9951..395bead651 100644 --- a/unittests/test_parameters.py +++ b/unittests/test_parameters.py @@ -10,6 +10,7 @@ import reframe as rfm from reframe.core.exceptions import ReframeSyntaxError +import reframe.core.decorators as decorators class NoParams(rfm.RunOnlyRegressionTest): @@ -54,6 +55,24 @@ class MyTest(Abstract): assert MyTest.param_space['P1'] == ('b',) +def test_abstract_param_warning(monkeypatch): + monkeypatch.setattr(decorators, '_TREAT_WARNINGS_AS_ERRORS', True) + + class MyTest(Abstract): + pass + + with pytest.raises(ValueError) as execinfo: + decorators._validate_test(MyTest) + + assert "P0" in str(execinfo.value) + + +def test_abstract_check(): + @rfm.simple_test + class MyTest(Abstract): + pass + + def test_param_override(): class MyTest(TwoParams): P1 = parameter(['-']) From 6e34d16e3725816cb6c2e08e3d47b704a233ac61 Mon Sep 17 00:00:00 2001 From: Blanca Fuentes Date: Fri, 25 Oct 2024 15:57:24 +0000 Subject: [PATCH 4/6] Fix warning issuing --- reframe/core/decorators.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 1a7d02b7cf..f7f856a53c 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -104,7 +104,7 @@ def instantiate_all(self, reset_sysenv=0, external_vars=None): kwargs['reset_sysenv'] = reset_sysenv leaf_tests.append(test(*args, **kwargs)) except SkipTestError as e: - getlogger().warning( + getlogger().verbose( f'skipping test {test.__qualname__!r}: {e}' ) except Exception: From 8d4a47287f8e32654798ed12d2984147dcc7696d Mon Sep 17 00:00:00 2001 From: Blanca Fuentes Date: Fri, 25 Oct 2024 16:00:56 +0000 Subject: [PATCH 5/6] Remove unused test --- unittests/test_parameters.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/unittests/test_parameters.py b/unittests/test_parameters.py index 395bead651..fcceb5197d 100644 --- a/unittests/test_parameters.py +++ b/unittests/test_parameters.py @@ -67,12 +67,6 @@ class MyTest(Abstract): assert "P0" in str(execinfo.value) -def test_abstract_check(): - @rfm.simple_test - class MyTest(Abstract): - pass - - def test_param_override(): class MyTest(TwoParams): P1 = parameter(['-']) From f96184104e4da4ce82075592752d539fb5430f05 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 13 Nov 2024 00:24:55 +0100 Subject: [PATCH 6/6] Address PR comments + style fixes + dead code elimination --- reframe/core/decorators.py | 17 ++--------------- reframe/core/fixtures.py | 15 ++------------- reframe/core/parameters.py | 9 +++++++-- unittests/test_fixtures.py | 4 +--- unittests/test_parameters.py | 17 ++++------------- 5 files changed, 16 insertions(+), 46 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index f7f856a53c..ff62aa5e60 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -8,7 +8,6 @@ # __all__ = ['simple_test'] -_TREAT_WARNINGS_AS_ERRORS = False import inspect import sys @@ -170,22 +169,10 @@ def _validate_test(cls): 'subclass of RegressionTest') if (cls.is_abstract()): - params = cls.param_space.params - undefined_params = [] - for param in params: - if params[param].is_abstract(): - undefined_params.append(param) - # Raise detailed warning - if _TREAT_WARNINGS_AS_ERRORS: - raise ValueError( - f'skipping test {cls.__qualname__!r}: ' + - f'test has the following undefined parameters: ' + - ', '.join(undefined_params) - ) getlogger().warning( f'skipping test {cls.__qualname__!r}: ' + - f'test has the following undefined parameters: ' + - ', '.join(undefined_params) + 'the following parameters are undefined: ' + + ', '.join(cls.param_space.undefined_params()) ) conditions = [VersionValidator(v) for v in cls._rfm_required_version] diff --git a/reframe/core/fixtures.py b/reframe/core/fixtures.py index 21f71b7585..3e790d3f46 100644 --- a/reframe/core/fixtures.py +++ b/reframe/core/fixtures.py @@ -325,12 +325,6 @@ def uninst_tests(self): '''Get the uninstantiated tests of this registry''' return self._registry.keys() - def _filter_valid_partitions(self, candidate_parts): - return [p for p in candidate_parts if p in self._env_by_part] - - def _filter_valid_environs(self, part, candidate_environs): - return [e for e in cadidate_environs if e in self._env_by_part[part]] - def _is_registry(self, other): if not isinstance(other, FixtureRegistry): raise TypeError('other is not a FixtureRegistry') @@ -775,14 +769,9 @@ def __init__(self, cls, *, scope='test', action='fork', variants='all', # Check that the fixture class is not an abstract test. if cls.is_abstract(): - params = cls.param_space.params - undefined_params = [] - for param in params: - if params[param].is_abstract(): - undefined_params.append(param) raise ValueError( - f'class {cls.__qualname__!r} has undefined parameters: ' + - ', '.join(undefined_params) + f'fixture {cls.__qualname__!r} has undefined parameters: ' + + ', '.join(cls.param_space.undefined_params()) ) # Validate the scope diff --git a/reframe/core/parameters.py b/reframe/core/parameters.py index 1633541971..f829b20e81 100644 --- a/reframe/core/parameters.py +++ b/reframe/core/parameters.py @@ -198,7 +198,7 @@ def update(self, other): self.values = tuple(filt_vals) + self.values except TypeError: raise ReframeSyntaxError( - f"'filter_param' must return an iterable" + "'filter_param' must return an iterable" ) from None def is_abstract(self): @@ -307,7 +307,7 @@ def inject(self, obj, cls=None, params_index=None): try: # Get the parameter values for the specified variant param_values = self.__param_combinations[params_index] - except IndexError as no_params: + except IndexError: raise RuntimeError( f'parameter space index out of range for ' f'{obj.__class__.__qualname__}' @@ -333,6 +333,11 @@ def defines(self, name): ''' return name in self.params and not self.params[name].is_abstract() + def undefined_params(self): + '''Return a list of all undefined parameters.''' + return [name for name, param in self.params.items() + if param.is_abstract()] + def __iter__(self): '''Create a generator object to iterate over the parameter space diff --git a/unittests/test_fixtures.py b/unittests/test_fixtures.py index 72c99e0144..2fe388a8f9 100644 --- a/unittests/test_fixtures.py +++ b/unittests/test_fixtures.py @@ -71,12 +71,10 @@ def test_abstract_fixture(): class Foo(rfm.RegressionTest): p = parameter() - with pytest.raises(ValueError) as exc_info: + with pytest.raises(ValueError): class MyTest(rfm.RegressionMixin): f = fixture(Foo) - assert "p" in str(exc_info.value) - def test_fixture_variants(): '''Fixtures must have at least one valid variant.''' diff --git a/unittests/test_parameters.py b/unittests/test_parameters.py index fcceb5197d..a818a3d8c3 100644 --- a/unittests/test_parameters.py +++ b/unittests/test_parameters.py @@ -49,22 +49,13 @@ class MyTest(TwoParams): def test_abstract_param(): class MyTest(Abstract): - pass + # Add another abstract parameter + P2 = parameter() assert MyTest.param_space['P0'] == () assert MyTest.param_space['P1'] == ('b',) - - -def test_abstract_param_warning(monkeypatch): - monkeypatch.setattr(decorators, '_TREAT_WARNINGS_AS_ERRORS', True) - - class MyTest(Abstract): - pass - - with pytest.raises(ValueError) as execinfo: - decorators._validate_test(MyTest) - - assert "P0" in str(execinfo.value) + assert MyTest.param_space['P2'] == () + assert MyTest.param_space.undefined_params() == ['P0', 'P2'] def test_param_override():