From e05f74979a2f8bd426eb6a44dc1d61ba50a8bae3 Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Wed, 11 Aug 2021 17:10:42 +0200 Subject: [PATCH 1/5] Relax filter_params requirements --- reframe/core/parameters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reframe/core/parameters.py b/reframe/core/parameters.py index 6f678269be..df1b3cb2cc 100644 --- a/reframe/core/parameters.py +++ b/reframe/core/parameters.py @@ -125,7 +125,7 @@ def extend(self, cls): local_param_space = getattr(cls, self.local_namespace_name) for name, p in local_param_space.items(): self.params[name] = ( - p.filter_params(self.params.get(name, ())) + p.values + tuple(p.filter_params(self.params.get(name, ()))) + p.values ) # If any previously declared parameter was defined in the class body From f50bdbf645cf7d4343d23323290b3ccc5c92ec7b Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Wed, 11 Aug 2021 18:03:16 +0200 Subject: [PATCH 2/5] Update parameter docs --- docs/regression_test_api.rst | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 3818559127..5ea06a82f3 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -47,6 +47,7 @@ Built-in types .. py:function:: RegressionMixin.parameter(values=None, inherit_params=False, filter_params=None) Inserts or modifies a regression test parameter. + At the class level, these parameters are stored in a separate namespace referred to as the *parameter space*. If a parameter with a matching name is already present in the parameter space of a parent class, the existing parameter values will be combined with those provided by this method following the inheritance behavior set by the arguments ``inherit_params`` and ``filter_params``. Instead, if no parameter with a matching name exists in any of the parent parameter spaces, a new regression test parameter is created. A regression test can be parameterized as follows: @@ -79,14 +80,33 @@ Built-in types else: override_other() + Moreover, a derived class may extend, partially extend and/or modify the parameter values provided in the base class as shown below. - :param values: A list containing the parameter values. - If no values are passed when creating a new parameter, the parameter is considered as *declared* but not *defined* (i.e. an abstract parameter). - Instead, for an existing parameter, this depends on the parameter's inheritance behaviour and on whether any values where provided in any of the parent parameter spaces. - :param inherit_params: If :obj:`False`, no parameter values that may have been defined in any of the parent parameter spaces will be inherited. + .. code:: python + + class ExtendVariant(Bar): + # Extend the variant parameter values to ['A', 'B', 'C'] + variant = parameter(['C'], inherit_params=True) + + class PartiallyExtendVariant(Bar): + # Extend the variant parameter values to ['A', 'D'] + variant = parameter(['D'], inherit_params=True, + filter_params=lambda x: x[:1]) + + class ModifyVariant(Bar): + # Modify the variant parameter values to ['AA', 'BA'] + variant = parameter(inherit_params=True, + filter_params=lambda x: map(lambda y: y+'A', x)) + + + :param values: An iterable containing the parameter values. + If no values are provided when creating a new parameter, the parameter is considered as *declared* but not *defined* (i.e. an abstract parameter). + On the other hand, if the parameter was defined in any of the base classes, the parameter may inherit the values provided in the parent class, and any values provided in the current class may effectively extend the values for this parameter. + :param inherit_params: If :obj:`True`, any parameter values defined in any base class will be inherited. + However, if this argument is set as :obj:`True` and the parameter does not exist in any of the parent parameter spaces, it has the same effect as setting this option to :obj:`False`. :param filter_params: Function to filter/modify the inherited parameter values that may have been provided in any of the parent parameter spaces. This function must accept a single argument, which will be passed as an iterable containing the inherited parameter values. - This only has an effect if used with ``inherit_params=True``. + This function must also return an iterable, and it will only have an effect if used with ``inherit_params=True``. .. py:function:: RegressionMixin.variable(*types, value=None) From ee0986c44ae2382559f7e437daaf12909a2b8dbe Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Wed, 11 Aug 2021 18:21:23 +0200 Subject: [PATCH 3/5] Catch TypeError and add unit tests --- reframe/core/parameters.py | 14 +++++++++++--- unittests/test_parameters.py | 15 ++++++++++++++- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/reframe/core/parameters.py b/reframe/core/parameters.py index df1b3cb2cc..f72962fc9c 100644 --- a/reframe/core/parameters.py +++ b/reframe/core/parameters.py @@ -124,9 +124,17 @@ def extend(self, cls): local_param_space = getattr(cls, self.local_namespace_name) for name, p in local_param_space.items(): - self.params[name] = ( - tuple(p.filter_params(self.params.get(name, ()))) + p.values - ) + try: + self.params[name] = ( + tuple(p.filter_params(self.params.get(name, ()))) + + p.values + ) + except TypeError: + raise ReframeSyntaxError( + f"argument 'filter_param' must be a callable taking a " + f"single argument that returns an iterable " + f"(parameter {name!r})" + ) # If any previously declared parameter was defined in the class body # by directly assigning it a value, raise an error. Parameters must be diff --git a/unittests/test_parameters.py b/unittests/test_parameters.py index dfc5556a06..a779ddac6a 100644 --- a/unittests/test_parameters.py +++ b/unittests/test_parameters.py @@ -72,13 +72,26 @@ class MyTest(TwoParams): def test_filter_params(): class MyTest(ExtendParams): - P1 = parameter(inherit_params=True, filter_params=lambda x: x[2:]) + # We make the return type of the filtering function a list to ensure + # that other iterables different to tuples are also valid. + P1 = parameter(inherit_params=True, + filter_params=lambda x: list(x[2:])) assert MyTest.param_space['P0'] == ('a',) assert MyTest.param_space['P1'] == ('d', 'e',) assert MyTest.param_space['P2'] == ('f', 'g',) +def test_wrong_filter(): + with pytest.raises(ReframeSyntaxError): + class Foo(ExtendParams): + P1 = parameter(inherit_params=True, filter_params=lambda x: 1) + + with pytest.raises(ReframeSyntaxError): + class Bar(ExtendParams): + P1 = parameter(inherit_params=True, filter_params=lambda x, y: []) + + def test_is_abstract_test(): class MyTest(Abstract): pass From c9a94626d544508a81b19681d781bcde86261ce8 Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Wed, 18 Aug 2021 11:39:45 +0200 Subject: [PATCH 4/5] Address PR comments --- docs/regression_test_api.rst | 27 +++++++++++++++++++-------- reframe/core/parameters.py | 21 +++++++++++---------- unittests/test_parameters.py | 11 +++++++---- 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 5ea06a82f3..e71dbab8be 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -85,11 +85,11 @@ Built-in types .. code:: python class ExtendVariant(Bar): - # Extend the variant parameter values to ['A', 'B', 'C'] + # Extend the full set of inherited variant parameter values to ['A', 'B', 'C'] variant = parameter(['C'], inherit_params=True) class PartiallyExtendVariant(Bar): - # Extend the variant parameter values to ['A', 'D'] + # Extend a subset of the inherited variant parameter values to ['A', 'D'] variant = parameter(['D'], inherit_params=True, filter_params=lambda x: x[:1]) @@ -98,15 +98,26 @@ Built-in types variant = parameter(inherit_params=True, filter_params=lambda x: map(lambda y: y+'A', x)) + A parameter with no values is referred to as an *abstract parameter* (i.e. a parameter that is declared but not defined). + Therefore, classes with at least one abstract parameter are considered abstract classes. + + .. code:: python + + class AbstractA(Bar): + variant = parameter() + + class AbstractB(Bar): + variant = parameter(inherit_params=True, filter_params=lambda x: []) + :param values: An iterable containing the parameter values. - If no values are provided when creating a new parameter, the parameter is considered as *declared* but not *defined* (i.e. an abstract parameter). - On the other hand, if the parameter was defined in any of the base classes, the parameter may inherit the values provided in the parent class, and any values provided in the current class may effectively extend the values for this parameter. - :param inherit_params: If :obj:`True`, any parameter values defined in any base class will be inherited. - However, if this argument is set as :obj:`True` and the parameter does not exist in any of the parent parameter spaces, it has the same effect as setting this option to :obj:`False`. + :param inherit_params: If :obj:`True`, the parameter values defined in any base class will be inherited. + In this case, the parameter values provided in the current class will extend the set of inherited parameter values. + If the parameter does not exist in any of the parent parameter spaces, this option has no effect. :param filter_params: Function to filter/modify the inherited parameter values that may have been provided in any of the parent parameter spaces. - This function must accept a single argument, which will be passed as an iterable containing the inherited parameter values. - This function must also return an iterable, and it will only have an effect if used with ``inherit_params=True``. + This function must accept a single iterable argument and return an iterable. + It will be called with the inherited parameter values and it must return the filtered set of parameter values. + This function will only have an effect if used with ``inherit_params=True``. .. py:function:: RegressionMixin.variable(*types, value=None) diff --git a/reframe/core/parameters.py b/reframe/core/parameters.py index f72962fc9c..380543e1ba 100644 --- a/reframe/core/parameters.py +++ b/reframe/core/parameters.py @@ -125,16 +125,17 @@ def extend(self, cls): local_param_space = getattr(cls, self.local_namespace_name) for name, p in local_param_space.items(): try: - self.params[name] = ( - tuple(p.filter_params(self.params.get(name, ()))) + - p.values - ) - except TypeError: - raise ReframeSyntaxError( - f"argument 'filter_param' must be a callable taking a " - f"single argument that returns an iterable " - f"(parameter {name!r})" - ) + filt_vals = p.filter_params(self.params.get(name, ())) + except Exception as err: + raise err + else: + try: + self.params[name] = (tuple(filt_vals) + p.values) + except TypeError: + raise ReframeSyntaxError( + f"'filter_param' must return an iterable " + f"(parameter {name!r})" + ) # If any previously declared parameter was defined in the class body # by directly assigning it a value, raise an error. Parameters must be diff --git a/unittests/test_parameters.py b/unittests/test_parameters.py index a779ddac6a..6a2903eb53 100644 --- a/unittests/test_parameters.py +++ b/unittests/test_parameters.py @@ -83,13 +83,16 @@ class MyTest(ExtendParams): def test_wrong_filter(): - with pytest.raises(ReframeSyntaxError): + def bad_filter(x): + raise RuntimeError('bad filter') + + with pytest.raises(RuntimeError): class Foo(ExtendParams): - P1 = parameter(inherit_params=True, filter_params=lambda x: 1) + P1 = parameter(inherit_params=True, filter_params=bad_filter) with pytest.raises(ReframeSyntaxError): - class Bar(ExtendParams): - P1 = parameter(inherit_params=True, filter_params=lambda x, y: []) + class Foo(ExtendParams): + P1 = parameter(inherit_params=True, filter_params=lambda x: 1) def test_is_abstract_test(): From 1492549dd3976712e856268dec9b49eba1f3fed7 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 18 Aug 2021 12:06:59 +0200 Subject: [PATCH 5/5] Fix minor style issues --- reframe/core/parameters.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/reframe/core/parameters.py b/reframe/core/parameters.py index 380543e1ba..f5b242f6ec 100644 --- a/reframe/core/parameters.py +++ b/reframe/core/parameters.py @@ -126,8 +126,8 @@ def extend(self, cls): for name, p in local_param_space.items(): try: filt_vals = p.filter_params(self.params.get(name, ())) - except Exception as err: - raise err + except Exception: + raise else: try: self.params[name] = (tuple(filt_vals) + p.values) @@ -135,7 +135,7 @@ def extend(self, cls): raise ReframeSyntaxError( f"'filter_param' must return an iterable " f"(parameter {name!r})" - ) + ) from None # If any previously declared parameter was defined in the class body # by directly assigning it a value, raise an error. Parameters must be