From a728dd68ed05e3c0ad4a13371a6fc9eaa1a336f3 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 26 Oct 2022 00:38:49 +0200 Subject: [PATCH 1/7] Introduce variable aliases This practically extends on the field aliasing that we used to have and brings the same concept to variables. Users can now define a variable as an alias of another one. This is quite useful for transitional periods and deprecations when a variable is renamed. This commit also introduces implementation improvements on how variables are inherited. --- reframe/core/meta.py | 6 +- reframe/core/variables.py | 136 ++++++++++++++++++++++++++++-------- unittests/test_variables.py | 48 ++++++++++++- 3 files changed, 156 insertions(+), 34 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index f3e151b825..1d2aa75afc 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -140,11 +140,9 @@ def __getitem__(self, key): # available in the current class' namespace. for b in self['_rfm_bases']: if key in b._rfm_var_space: - # Store a deep-copy of the variable's - # value and return. - v = b._rfm_var_space[key].default_value + v = variables.ShadowVar(b._rfm_var_space[key]) self._namespace[key] = v - return self._namespace[key] + return v # If 'key' is neither a variable nor a parameter, # raise the exception from the base __getitem__. diff --git a/reframe/core/variables.py b/reframe/core/variables.py index b90bf97e68..6a53ea319b 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -172,16 +172,40 @@ class MyRequiredTest(HelloTest): the field validator. :returns: A new test variable. - .. versionadded:: 3.10.2 + .. version added:: 3.10.2 The ``loggable`` argument is added. ''' - __slots__ = ('_default_value', '_field', '_loggable', '_name') + # NOTE: We can't use truly private fields in __slots__, because + # __setattr__() will be called with their mangled name and we cannot match + # them in the __slots__ with making implementation-defined assumptions + # about the mangled name. So we just add the `_p_` prefix for "private" + + __slots__ = ('_p_default_value', '_p_field', + '_loggable', '_name', '_target') + + __mutable_props = ('_default_value',) def __init__(self, *args, **kwargs): field_type = kwargs.pop('field', fields.TypedField) - self._default_value = kwargs.pop('value', Undefined) + alias = kwargs.pop('alias', None) + + if alias and not isinstance(alias, TestVar): + raise TypeError(f"'alias' must refer to a variable; " + f"found {type(alias).__name__!r}") + + if alias and 'field' in kwargs: + raise ValueError(f"'field' cannot be set for an alias variable") + + if alias and 'value' in kwargs: + raise ValueError('alias variables do not accept default values') + + if alias: + self._p_default_value = alias._default_value + else: + self._p_default_value = kwargs.pop('value', Undefined) + self._loggable = kwargs.pop('loggable', False) if not issubclass(field_type, fields.Field): @@ -190,16 +214,22 @@ def __init__(self, *args, **kwargs): f'{fields.Field.__qualname__}' ) - self._field = field_type(*args, **kwargs) + if alias: + self._p_field = alias._field + else: + self._p_field = field_type(*args, **kwargs) + + self._target = alias @classmethod def create_deprecated(cls, var, message, kind=DEPRECATE_RDWR, from_version='0.0.0'): ret = TestVar.__new__(TestVar) - ret._field = fields.DeprecatedField(var.field, message, - kind, from_version) - ret._default_value = var._default_value + ret._p_field = fields.DeprecatedField(var.field, message, + kind, from_version) + ret._p_default_value = var._default_value ret._loggable = var._loggable + ret._target = var._target return ret def _check_deprecation(self, kind): @@ -217,21 +247,23 @@ def undefine(self): self._default_value = Undefined def define(self, value): - if value != self._default_value: - # We only issue a deprecation warning if the write attempt changes - # the value. This is a workaround to the fact that if a variable - # defined in parent classes is accessed by the current class, then - # the definition of the variable is "copied" in the class body as - # an assignment (see `MetaNamespace.__getitem__()`). The - # `VarSpace.extend()` method then checks all local class body - # assignments and if they refer to a variable (inherited or not), - # they call `define()` on it. So, practically, in this case, the - # `_default_value` is set redundantly once per class in the - # hierarchy. - self._check_deprecation(DEPRECATE_WR) - + self._check_deprecation(DEPRECATE_WR) self._default_value = value + @property + def _default_value(self): + if self._target: + return self._target._default_value + else: + return self._p_default_value + + @_default_value.setter + def _default_value(self, value): + if self._target: + self._target._default_value = value + else: + self._p_default_value = value + @property def default_value(self): # Variables must be returned by-value to prevent an instance from @@ -240,6 +272,13 @@ def default_value(self): self._check_deprecation(DEPRECATE_RD) return copy.deepcopy(self._default_value) + @property + def _field(self): + if self._target: + return self._target._field + else: + return self._p_field + @property def field(self): return self._field @@ -253,14 +292,14 @@ def __set_name__(self, owner, name): def __setattr__(self, name, value): '''Set any additional variable attribute into the default value.''' - if name in self.__slots__: + if name in self.__slots__ or name in self.__mutable_props: super().__setattr__(name, value) else: setattr(self._default_value, name, value) def __getattr__(self, name): '''Attribute lookup into the variable's value.''' - def_val = self.__getattribute__('_default_value') + def_val = self.__getattribute__('_p_default_value') # NOTE: This if below is necessary to avoid breaking the deepcopy # of instances of this class. Without it, a deepcopy of instances of @@ -284,12 +323,22 @@ def _check_is_defined(self): f'variable {self._name!r} is not assigned a value' ) - def __repr__(self): + def __str__(self): self._check_is_defined() - return repr(self._default_value) + return str(self._default_value) - def __str__(self): - return self.__repr__() + def __repr__(self): + try: + name = self.name + except AttributeError: + name = '' + + if self.is_defined(): + value = self._default_value + else: + value = '' + + return f'TestVar(name={name!r}, value={value!r})' def __bytes__(self): self._check_is_defined() @@ -593,6 +642,30 @@ def __ceil__(self): return math.ceil(self._default_value) +class ShadowVar(TestVar): + '''A shadow instance of another variable. + + This is essentially a fully-fledged shallow copy of another variable. It + is used during the construction of the class namespace to bring in scope a + requested variable that is defined in a base class (see + `MetaNamespace.__getitem__()`) + + We could not simply create a reference of the original variable in the + current namespace, because we need a mechanism to differentiate the + lowered variable from any redefinition, which is illegal. + + Also, we don't need a deep copy, since the shadow variable will replace + the original variable in the newly constructed `VarSpace`. + + ''' + + def __init__(self, other): + for name in self.__slots__: + setattr(self, name, getattr(other, name)) + + self._check_deprecation(DEPRECATE_RD) + + class VarSpace(namespaces.Namespace): '''Variable space of a regression test. @@ -645,8 +718,7 @@ def extend(self, cls): is disallowed. ''' local_varspace = getattr(cls, self.local_namespace_name, False) - while local_varspace: - key, var = local_varspace.popitem() + for key, var in local_varspace.items(): if isinstance(var, TestVar): # Disable redeclaring a variable if key in self.vars: @@ -657,13 +729,19 @@ def extend(self, cls): # Add a new var self.vars[key] = var + local_varspace.clear() + # If any previously declared variable was defined in the class body # by directly assigning it a value, retrieve this value from the class # namespace and update it into the variable space. _assigned_vars = set() for key, value in cls.__dict__.items(): if key in self.vars: - self.vars[key].define(value) + if isinstance(value, ShadowVar): + self.vars[key] = value + else: + self.vars[key].define(value) + _assigned_vars.add(key) elif value is Undefined: # Cannot be set as Undefined if not a variable diff --git a/unittests/test_variables.py b/unittests/test_variables.py index 3d755aa99c..860d5047dd 100644 --- a/unittests/test_variables.py +++ b/unittests/test_variables.py @@ -219,7 +219,7 @@ class Foo(rfm.RegressionMixin): with pytest.raises(ReframeSyntaxError): class Foo(rfm.RegressionMixin): my_var = variable(int) - x = f'accessing {my_var!r} fails because its value is not set.' + x = f'accessing {my_var} fails because its value is not set.' def test_var_space_is_read_only(): @@ -486,3 +486,49 @@ class D(A): c = a.y with pytest.warns(ReframeDeprecationWarning): a.y = 10 + + +def test_var_aliases(): + with pytest.raises(ValueError, + match=r'alias variables do not accept default values'): + class T(rfm.RegressionMixin): + x = variable(int, value=1) + y = variable(alias=x, value=2) + + class T(rfm.RegressionMixin): + x = variable(int, value=1) + y = variable(alias=x) + z = variable(alias=y) + + t = T() + assert t.y == 1 + assert t.z == 1 + + t.x = 4 + assert t.y == 4 + assert t.z == 4 + + t.y = 5 + assert t.x == 5 + assert t.z == 5 + + t.z = 10 + assert t.x == 10 + assert t.y == 10 + + # Test inheritance + + class T(rfm.RegressionMixin): + x = variable(int, value=1) + + class S(T): + y = variable(alias=x) + + s = S() + assert s.y == 1 + + s.y = 2 + assert s.x == 2 + + s.x = 3 + assert s.y == 3 From 3a4c093580c2659f8dce79abbe01f615a24f3a39 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 27 Oct 2022 22:49:19 +0200 Subject: [PATCH 2/7] Expand unit tests --- reframe/core/variables.py | 20 ++++++++++---------- unittests/test_variables.py | 12 ++++++++++++ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/reframe/core/variables.py b/reframe/core/variables.py index 6a53ea319b..d80c2fc8f9 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -177,10 +177,11 @@ class MyRequiredTest(HelloTest): ''' - # NOTE: We can't use truly private fields in __slots__, because - # __setattr__() will be called with their mangled name and we cannot match - # them in the __slots__ with making implementation-defined assumptions - # about the mangled name. So we just add the `_p_` prefix for "private" + # NOTE: We can't use truly private fields in `__slots__`, because + # `__setattr__()` will be called with their mangled name and we cannot + # match them in the `__slots__` without making implementation-defined + # assumptions about the mangled name. So we just add the `_p_` prefix for + # to denote the "private" fields. __slots__ = ('_p_default_value', '_p_field', '_loggable', '_name', '_target') @@ -188,19 +189,18 @@ class MyRequiredTest(HelloTest): __mutable_props = ('_default_value',) def __init__(self, *args, **kwargs): - field_type = kwargs.pop('field', fields.TypedField) alias = kwargs.pop('alias', None) - - if alias and not isinstance(alias, TestVar): - raise TypeError(f"'alias' must refer to a variable; " - f"found {type(alias).__name__!r}") - if alias and 'field' in kwargs: raise ValueError(f"'field' cannot be set for an alias variable") if alias and 'value' in kwargs: raise ValueError('alias variables do not accept default values') + if alias and not isinstance(alias, TestVar): + raise TypeError(f"'alias' must refer to a variable; " + f"found {type(alias).__name__!r}") + + field_type = kwargs.pop('field', fields.TypedField) if alias: self._p_default_value = alias._default_value else: diff --git a/unittests/test_variables.py b/unittests/test_variables.py index 860d5047dd..850e856fe4 100644 --- a/unittests/test_variables.py +++ b/unittests/test_variables.py @@ -495,6 +495,18 @@ class T(rfm.RegressionMixin): x = variable(int, value=1) y = variable(alias=x, value=2) + with pytest.raises(TypeError, match=r"'alias' must refer to a variable"): + class T(rfm.RegressionMixin): + x = variable(int, value=1) + y = variable(alias=10) + + with pytest.raises(ValueError, match=r"'field' cannot be set"): + from reframe.core.fields import TypedField + + class T(rfm.RegressionMixin): + x = variable(int, value=1) + y = variable(alias=x, field=TypedField) + class T(rfm.RegressionMixin): x = variable(int, value=1) y = variable(alias=x) From 09b9cc8b38d2df2ae548c47a0a1a0bacc7102310 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 27 Oct 2022 23:00:04 +0200 Subject: [PATCH 3/7] Document alias variables --- reframe/core/variables.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/reframe/core/variables.py b/reframe/core/variables.py index d80c2fc8f9..3a96108f30 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -158,6 +158,14 @@ class MyRequiredTest(HelloTest): from :class:`EchoBaseTest` to throw an error indicating that the variable ``what`` has not been set. + Finally, variables may alias each other. If a variable is an alias of + another one it behaves in the exact same way as its target. If a change is + made to the target variable, this is reflected to the alias and vice + versa. However, alias variables are independently loggable: an alias may + be logged but not its target and vice versa. Aliased variables are useful + when you want to rename a variable and you want to keep the old one for + compatibility reasons. + :param `types`: the supported types for the variable. :param value: the default value assigned to the variable. If no value is provided, the variable is set as ``required``. @@ -165,6 +173,9 @@ class MyRequiredTest(HelloTest): field argument is provided, it defaults to :attr:`reframe.core.fields.TypedField`. The provided field validator by this argument must derive from :attr:`reframe.core.fields.Field`. + :param alias: the target variable if this variable is an alias. This must + refer to an already declared variable and neither default value nor a + field can be specified for an alias variable. :param loggable: Mark this variable as loggable. If :obj:`True`, this variable will become a log record attribute under the name ``check_NAME``, where ``NAME`` is the name of the variable. @@ -172,9 +183,12 @@ class MyRequiredTest(HelloTest): the field validator. :returns: A new test variable. - .. version added:: 3.10.2 + .. versionadded:: 3.10.2 The ``loggable`` argument is added. + .. versionadded:: 4.0.0 + Alias variable are introduced. + ''' # NOTE: We can't use truly private fields in `__slots__`, because From 157847303d7eea9723de2b78d104ee34e1197a76 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 27 Oct 2022 23:04:05 +0200 Subject: [PATCH 4/7] Check logging of aliased variables --- unittests/test_logging.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/unittests/test_logging.py b/unittests/test_logging.py index 08214fcbaa..075abbfdb2 100644 --- a/unittests/test_logging.py +++ b/unittests/test_logging.py @@ -29,6 +29,7 @@ def fake_check(): class _FakeCheck(rfm.RegressionTest): param = parameter(range(3), loggable=True, fmt=lambda x: 10*x) custom = variable(str, value='hello extras', loggable=True) + custom2 = variable(alias=custom) custom_list = variable(list, value=['custom', 3.0, ['hello', 'world']], loggable=True) @@ -161,13 +162,13 @@ def test_logger_levels(logfile, logger_with_check): def test_logger_loggable_attributes(logfile, logger_with_check): formatter = rlog.RFC3339Formatter( - '%(check_custom)s|%(check_custom_list)s|' + '%(check_custom)s|%(check_custom2)s|%(check_custom_list)s|' '%(check_foo)s|%(check_custom_dict)s|%(check_param)s|%(check_x)s' ) logger_with_check.logger.handlers[0].setFormatter(formatter) logger_with_check.info('xxx') assert _pattern_in_logfile( - r'hello extras\|custom,3.0,\["hello", "world"\]\|null\|' + r'hello extras\|null\|custom,3.0,\["hello", "world"\]\|null\|' r'{"a": 1, "b": 2}\|10\|null', logfile ) From 1a8b334ae136ca2986ebc70300a4a8abbe7f5200 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 27 Oct 2022 23:59:39 +0200 Subject: [PATCH 5/7] Treat correctly variable deprecations --- reframe/core/variables.py | 19 ++++++++++++------- unittests/test_variables.py | 14 +++++++++++++- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/reframe/core/variables.py b/reframe/core/variables.py index 3a96108f30..64267ff648 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -246,10 +246,12 @@ def create_deprecated(cls, var, message, ret._target = var._target return ret - def _check_deprecation(self, kind): - if isinstance(self.field, fields.DeprecatedField): - if self.field.op & kind: - user_deprecation_warning(self.field.message) + def _warn_deprecation(self, kind): + if self.is_deprecated() and self.field.op & kind: + user_deprecation_warning(self.field.message) + + def is_deprecated(self): + return isinstance(self._p_field, fields.DeprecatedField) def is_loggable(self): return self._loggable @@ -261,7 +263,7 @@ def undefine(self): self._default_value = Undefined def define(self, value): - self._check_deprecation(DEPRECATE_WR) + self._warn_deprecation(DEPRECATE_WR) self._default_value = value @property @@ -283,11 +285,14 @@ def default_value(self): # Variables must be returned by-value to prevent an instance from # modifying the class variable space. self._check_is_defined() - self._check_deprecation(DEPRECATE_RD) + self._warn_deprecation(DEPRECATE_RD) return copy.deepcopy(self._default_value) @property def _field(self): + if self.is_deprecated(): + return self._p_field + if self._target: return self._target._field else: @@ -677,7 +682,7 @@ def __init__(self, other): for name in self.__slots__: setattr(self, name, getattr(other, name)) - self._check_deprecation(DEPRECATE_RD) + self._warn_deprecation(DEPRECATE_RD) class VarSpace(namespaces.Namespace): diff --git a/unittests/test_variables.py b/unittests/test_variables.py index 850e856fe4..8a777fabf3 100644 --- a/unittests/test_variables.py +++ b/unittests/test_variables.py @@ -9,6 +9,7 @@ import reframe as rfm from reframe.core.exceptions import ReframeSyntaxError +from reframe.core.warnings import ReframeDeprecationWarning @pytest.fixture @@ -458,7 +459,6 @@ class A(rfm.RegressionTest): def test_var_deprecation(): from reframe.core.variables import DEPRECATE_RD, DEPRECATE_WR - from reframe.core.warnings import ReframeDeprecationWarning # Check read deprecation class A(rfm.RegressionMixin): @@ -544,3 +544,15 @@ class S(T): s.x = 3 assert s.y == 3 + + # Test deprecated aliases + class S(T): + y = deprecate(variable(alias=x), f'y is deprecated') + + with pytest.warns(ReframeDeprecationWarning): + class U(S): + y = 10 + + s = S() + with pytest.warns(ReframeDeprecationWarning): + s.y = 10 From d97049797f3b2dfa2a3f12bc6d209975b31fbfc6 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 28 Oct 2022 00:18:07 +0200 Subject: [PATCH 6/7] Stricter test of alias value --- reframe/core/variables.py | 11 +++++------ unittests/test_variables.py | 6 +++--- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/reframe/core/variables.py b/reframe/core/variables.py index 64267ff648..ed22c8452a 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -204,31 +204,30 @@ class MyRequiredTest(HelloTest): def __init__(self, *args, **kwargs): alias = kwargs.pop('alias', None) - if alias and 'field' in kwargs: + if alias is not None and 'field' in kwargs: raise ValueError(f"'field' cannot be set for an alias variable") - if alias and 'value' in kwargs: + if alias is not None and 'value' in kwargs: raise ValueError('alias variables do not accept default values') - if alias and not isinstance(alias, TestVar): + if alias is not None and not isinstance(alias, TestVar): raise TypeError(f"'alias' must refer to a variable; " f"found {type(alias).__name__!r}") field_type = kwargs.pop('field', fields.TypedField) - if alias: + if alias is not None: self._p_default_value = alias._default_value else: self._p_default_value = kwargs.pop('value', Undefined) self._loggable = kwargs.pop('loggable', False) - if not issubclass(field_type, fields.Field): raise TypeError( f'field {field_type!r} is not derived from ' f'{fields.Field.__qualname__}' ) - if alias: + if alias is not None: self._p_field = alias._field else: self._p_field = field_type(*args, **kwargs) diff --git a/unittests/test_variables.py b/unittests/test_variables.py index 8a777fabf3..909f36abc2 100644 --- a/unittests/test_variables.py +++ b/unittests/test_variables.py @@ -508,13 +508,13 @@ class T(rfm.RegressionMixin): y = variable(alias=x, field=TypedField) class T(rfm.RegressionMixin): - x = variable(int, value=1) + x = variable(int, value=0) y = variable(alias=x) z = variable(alias=y) t = T() - assert t.y == 1 - assert t.z == 1 + assert t.y == 0 + assert t.z == 0 t.x = 4 assert t.y == 4 From 6af752222039b0fbc30941ef5e763310852a5296 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 28 Oct 2022 21:19:01 +0200 Subject: [PATCH 7/7] repr() workaround for Sphinx docs --- docs/conf.py | 10 ++++++++++ reframe/core/variables.py | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/docs/conf.py b/docs/conf.py index 3ff256ade5..43a73bca9b 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -38,6 +38,16 @@ import reframe # noqa: F401, F403 import reframe.utility.osext as osext # noqa: F401, F403 +# Sphinx erroneously uses `repr()` to display values and not the +# human-readable `str()`. For this reason we define the following variable in +# the `reframe` module so that objects can redirect to their `str()` method +# from `repr()` if they are invoked in the context of Sphinx when building the +# documentation. See discussion and the related workaround here: +# +# https://github.com/sphinx-doc/sphinx/issues/3857 + +reframe.__build_docs__ = True + # -- General configuration ------------------------------------------------ # If your documentation needs a minimal Sphinx version, state it here. diff --git a/reframe/core/variables.py b/reframe/core/variables.py index ed22c8452a..961744e850 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -346,6 +346,10 @@ def __str__(self): return str(self._default_value) def __repr__(self): + import reframe + if hasattr(reframe, '__build_docs__'): + return str(self) + try: name = self.name except AttributeError: