diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 9b839362c7..4635da5c43 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -111,10 +111,10 @@ So although a "post-init" and a "pre-setup" hook will both run *after* a test ha the post-init hook will execute *right after* the test is initialized. The framework will then continue with other activities and it will execute the pre-setup hook *just before* it schedules the test for executing its setup stage. -Pipeline hooks are executed in reverse MRO order, i.e., the hooks of the least specialized class will be executed first. +Pipeline hooks are normally executed in reverse MRO order, i.e., the hooks of the least specialized class will be executed first. In the following example, :func:`BaseTest.x` will execute before :func:`DerivedTest.y`: -.. code:: python +.. code-block:: python class BaseTest(rfm.RegressionTest): @run_after('setup') @@ -127,6 +127,34 @@ In the following example, :func:`BaseTest.x` will execute before :func:`DerivedT '''Hook y''' +This order can be altered using the ``always_last`` argument of the :func:`@run_before ` and :func:`@run_after ` decorators. +In this case, all hooks of the same stage defined with ``always_last=True`` will be executed in MRO order at the end of the stage's hook chain. +For example, given the following hierarchy: + + .. code-block:: python + + class X(rfm.RunOnlyRegressionTest): + @run_before('run', always_last=True) + def hook_a(self): pass + + @run_before('run') + def hook_b(self): pass + + class Y(X): + @run_before('run', always_last=True) + def hook_c(self): pass + + @run_before('run') + def hook_d(self): pass + + +the run hooks of :class:`Y` will be executed as follows: + +.. code-block:: console + + X.hook_b, Y.hook_d, Y.hook_c, X.hook_a + + .. seealso:: - :func:`@run_before `, :func:`@run_after ` decorators @@ -146,6 +174,12 @@ In the following example, :func:`BaseTest.x` will execute before :func:`DerivedT with osext.change_dir(self.stagedir): ... +.. note:: + In versions prior to 4.3.4, overriding a pipeline hook in a subclass would re-attach it from scratch in the stage, therefore changing its execution order relative to other hooks of the superclass. + This is fixed in versions >= 4.3.4 where the execution order of the hook is not changed. + However, the fix may break existing workaround code that used to call explicitly the base class' hook from the derived one. + Check issue `#3012 `__ for details on how to properly address this. + .. warning:: .. versionchanged:: 3.7.0 Declaring pipeline hooks using the same name functions from the :py:mod:`reframe` or :py:mod:`reframe.core.decorators` modules is now deprecated. diff --git a/reframe/core/builtins.py b/reframe/core/builtins.py index 6f32c3a7f4..4b5d4c8e4b 100644 --- a/reframe/core/builtins.py +++ b/reframe/core/builtins.py @@ -48,15 +48,18 @@ def run_before(stage, *, always_last=False): :param stage: The pipeline stage where this function will be attached to. See :ref:`pipeline-hooks` for the list of valid stage values. - :param always_last: Run this hook always as the last one of the stage. In - a whole test hierarchy, only a single hook can be explicitly pinned at - the end of the same-stage sequence of hooks. If another hook is - declared as ``always_last`` in the same stage, an error will be - issued. + :param always_last: Run this hook at the end of the stage's hook chain + instead of the beginning. If multiple tests set this flag for a hook + in the same stage, then all ``always_last`` hooks will be executed in + MRO order at the end of stage's hook chain. See :ref:`pipeline-hooks` + for an example execution. .. versionchanged:: 4.4 The ``always_last`` argument was added. + .. versionchanged:: 4.5 + Multiple tests can set ``always_last`` in the same stage. + ''' return hooks.attach_to('pre_' + stage, always_last) diff --git a/reframe/core/parameters.py b/reframe/core/parameters.py index c8659589b3..195cc21f9e 100644 --- a/reframe/core/parameters.py +++ b/reframe/core/parameters.py @@ -121,7 +121,8 @@ class AbstractB(Bar): :param loggable: Mark this parameter as loggable. If :obj:`True`, this parameter will become a log record attribute under the name - ``check_NAME``, where ``NAME`` is the name of the parameter. + ``check_NAME``, where ``NAME`` is the name of the parameter (default: + :obj:`True`) :returns: A new test parameter. @@ -130,10 +131,14 @@ class AbstractB(Bar): .. versionadded:: 3.11.0 The ``loggable`` argument is added. + + .. versionchanged:: 4.5 + Parameters are now loggable by default. + ''' def __init__(self, values=None, inherit_params=False, - filter_params=None, fmt=None, loggable=False): + filter_params=None, fmt=None, loggable=True): if values is None: values = [] diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 95a241be2c..28a73887d2 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -183,16 +183,10 @@ def pipeline_hooks(cls): for hook in cls._rfm_hook_registry: for stage, always_last in hook.stages: if always_last: - if stage in last: - hook_name = hook.__qualname__ - pinned_name = last[stage].__qualname__ - raise ReframeSyntaxError( - f'cannot pin hook {hook_name!r} as last ' - f'of stage {stage!r} as {pinned_name!r} ' - f'is already pinned last' - ) - - last[stage] = hook + try: + last[stage].append(hook.fn) + except KeyError: + last[stage] = [hook.fn] else: try: ret[stage].append(hook.fn) @@ -200,11 +194,12 @@ def pipeline_hooks(cls): ret[stage] = [hook.fn] # Append the last hooks - for stage, hook in last.items(): - try: - ret[stage].append(hook.fn) - except KeyError: - ret[stage] = [hook.fn] + for stage, hooks in last.items(): + for fn in reversed(hooks): + try: + ret[stage].append(fn) + except KeyError: + ret[stage] = [fn] return ret @@ -236,8 +231,7 @@ def pipeline_hooks(cls): #: #: .. versionchanged:: 3.11.0 #: Extend syntax to support features and key/value pairs. - valid_prog_environs = variable(typ.List[typ.Str[_VALID_ENV_SYNTAX]], - loggable=True) + valid_prog_environs = variable(typ.List[typ.Str[_VALID_ENV_SYNTAX]]) #: List of systems or system features or system properties required by this #: test. @@ -300,8 +294,7 @@ def pipeline_hooks(cls): #: #: .. versionchanged:: 3.11.0 #: Extend syntax to support features and key/value pairs. - valid_systems = variable(typ.List[typ.Str[_VALID_SYS_SYNTAX]], - loggable=True) + valid_systems = variable(typ.List[typ.Str[_VALID_SYS_SYNTAX]]) #: A detailed description of the test. #: @@ -328,7 +321,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`str` #: :default: ``''`` - sourcepath = variable(str, value='', loggable=True) + sourcepath = variable(str, value='') #: The directory containing the test's resources. #: @@ -358,7 +351,7 @@ def pipeline_hooks(cls): #: .. versionchanged:: 3.0 #: Default value is now conditionally set to either ``'src'`` or #: :class:`None`. - sourcesdir = variable(str, type(None), value='src', loggable=True) + sourcesdir = variable(str, type(None), value='src') #: .. versionadded:: 2.14 #: @@ -375,7 +368,8 @@ def pipeline_hooks(cls): #: #: :type: :class:`str` or :class:`reframe.core.buildsystems.BuildSystem`. #: :default: :class:`None`. - build_system = variable(type(None), field=BuildSystemField, value=None) + build_system = variable(type(None), field=BuildSystemField, value=None, + loggable=False) #: .. versionadded:: 3.0 #: @@ -387,7 +381,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`List[str]` #: :default: ``[]`` - prebuild_cmds = variable(typ.List[str], value=[], loggable=True) + prebuild_cmds = variable(typ.List[str], value=[]) #: .. versionadded:: 3.0 #: @@ -399,7 +393,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`List[str]` #: :default: ``[]`` - postbuild_cmds = variable(typ.List[str], value=[], loggable=True) + postbuild_cmds = variable(typ.List[str], value=[]) #: The name of the executable to be launched during the run phase. #: @@ -414,13 +408,13 @@ def pipeline_hooks(cls): #: .. versionchanged:: 3.7.3 #: Default value changed from ``os.path.join('.', self.unique_name)`` to #: :class:`required`. - executable = variable(str, loggable=True) + executable = variable(str) #: List of options to be passed to the :attr:`executable`. #: #: :type: :class:`List[str]` #: :default: ``[]`` - executable_opts = variable(typ.List[str], value=[], loggable=True) + executable_opts = variable(typ.List[str], value=[]) #: .. versionadded:: 2.20 #: @@ -460,7 +454,7 @@ def pipeline_hooks(cls): #: This field is now set automatically from the current partition's #: configuration. container_platform = variable(field=ContainerPlatformField, - value=_NoRuntime()) + value=_NoRuntime(), loggable=False) #: .. versionadded:: 3.0 #: @@ -472,7 +466,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`List[str]` #: :default: ``[]`` - prerun_cmds = variable(typ.List[str], value=[], loggable=True) + prerun_cmds = variable(typ.List[str], value=[]) #: .. versionadded:: 3.0 #: @@ -483,7 +477,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`List[str]` #: :default: ``[]`` - postrun_cmds = variable(typ.List[str], value=[], loggable=True) + postrun_cmds = variable(typ.List[str], value=[]) #: List of files to be kept after the test finishes. #: @@ -503,7 +497,7 @@ def pipeline_hooks(cls): #: .. versionchanged:: 3.3 #: This field accepts now also file glob patterns. #: - keep_files = variable(typ.List[str], value=[], loggable=True) + keep_files = variable(typ.List[str], value=[]) #: List of files or directories (relative to the :attr:`sourcesdir`) that #: will be symlinked in the stage directory and not copied. @@ -513,7 +507,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`List[str]` #: :default: ``[]`` - readonly_files = variable(typ.List[str], value=[], loggable=True) + readonly_files = variable(typ.List[str], value=[]) #: Set of tags associated with this test. #: @@ -521,7 +515,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`Set[str]` #: :default: an empty set - tags = variable(typ.Set[str], value=set(), loggable=True) + tags = variable(typ.Set[str], value=set()) #: List of people responsible for this test. #: @@ -529,7 +523,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`List[str]` #: :default: ``[]`` - maintainers = variable(typ.List[str], value=[], loggable=True) + maintainers = variable(typ.List[str], value=[]) #: Mark this test as a strict performance test. #: @@ -539,7 +533,7 @@ def pipeline_hooks(cls): #: #: :type: boolean #: :default: :class:`True` - strict_check = variable(typ.Bool, value=True, loggable=True) + strict_check = variable(typ.Bool, value=True) #: Number of tasks required by this test. #: @@ -582,7 +576,7 @@ def pipeline_hooks(cls): #: number of required tasks by the test. #: .. versionchanged:: 4.1 #: Allow :attr:`num_tasks` to be :obj:`None`. - num_tasks = variable(int, type(None), value=1, loggable=True) + num_tasks = variable(int, type(None), value=1) #: Number of tasks per node required by this test. #: @@ -590,7 +584,7 @@ def pipeline_hooks(cls): #: #: :type: integral or :class:`None` #: :default: :class:`None` - num_tasks_per_node = variable(int, type(None), value=None, loggable=True) + num_tasks_per_node = variable(int, type(None), value=None) #: Number of GPUs per node required by this test. #: This attribute is translated internally to the ``_rfm_gpu`` resource. @@ -602,7 +596,7 @@ def pipeline_hooks(cls): #: #: .. versionchanged:: 4.0.0 #: The default value changed to :const:`None`. - num_gpus_per_node = variable(int, type(None), value=None, loggable=True) + num_gpus_per_node = variable(int, type(None), value=None) #: Number of CPUs per task required by this test. #: @@ -610,7 +604,7 @@ def pipeline_hooks(cls): #: #: :type: integral or :class:`None` #: :default: :class:`None` - num_cpus_per_task = variable(int, type(None), value=None, loggable=True) + num_cpus_per_task = variable(int, type(None), value=None) #: Number of tasks per core required by this test. #: @@ -618,7 +612,7 @@ def pipeline_hooks(cls): #: #: :type: integral or :class:`None` #: :default: :class:`None` - num_tasks_per_core = variable(int, type(None), value=None, loggable=True) + num_tasks_per_core = variable(int, type(None), value=None) #: Number of tasks per socket required by this test. #: @@ -626,7 +620,7 @@ def pipeline_hooks(cls): #: #: :type: integral or :class:`None` #: :default: :class:`None` - num_tasks_per_socket = variable(int, type(None), value=None, loggable=True) + num_tasks_per_socket = variable(int, type(None), value=None) #: Specify whether this tests needs simultaneous multithreading enabled. #: @@ -634,8 +628,7 @@ def pipeline_hooks(cls): #: #: :type: boolean or :class:`None` #: :default: :class:`None` - use_multithreading = variable( - typ.Bool, type(None), value=None, loggable=True) + use_multithreading = variable(typ.Bool, type(None), value=None) #: .. versionadded:: 3.0 #: @@ -646,19 +639,19 @@ def pipeline_hooks(cls): #: :type: :class:`str` or :class:`datetime.timedelta` #: :default: :class:`None` max_pending_time = variable(type(None), typ.Duration, value=None, - loggable=True, allow_implicit=True) + allow_implicit=True) #: Specify whether this test needs exclusive access to nodes. #: #: :type: boolean #: :default: :class:`False` - exclusive_access = variable(typ.Bool, value=False, loggable=True) + exclusive_access = variable(typ.Bool, value=False) #: Always execute this test locally. #: #: :type: boolean #: :default: :class:`False` - local = variable(typ.Bool, value=False, loggable=True) + local = variable(typ.Bool, value=False) #: The set of reference values for this test. #: @@ -721,7 +714,7 @@ def pipeline_hooks(cls): typ.Tuple[~Deferrable, ~Deferrable, ~Deferrable], typ.Dict[str, typ.Dict[str, typ.Tuple[~Deferrable, ~Deferrable, ~Deferrable, ~Deferrable]]], - field=fields.ScopedDictField, value={} + field=fields.ScopedDictField, value={}, loggable=False ) #: Require that a reference is defined for each system that this test is @@ -734,7 +727,7 @@ def pipeline_hooks(cls): #: :default: :const:`False` #: #: .. versionadded:: 4.0.0 - require_reference = variable(typ.Bool, value=False) + require_reference = variable(typ.Bool, value=False, loggable=False) #: #: Refer to the :doc:`ReFrame Tutorials ` for concrete usage @@ -761,7 +754,7 @@ def pipeline_hooks(cls): #: #: .. versionchanged:: 3.6 #: The default value has changed from ``None`` to ``required``. - sanity_patterns = variable(_DeferredExpression) + sanity_patterns = variable(_DeferredExpression, loggable=False) #: Patterns for verifying the performance of this test. #: @@ -780,7 +773,8 @@ def pipeline_hooks(cls): #: ` builtin or the #: :attr:`perf_variables`, as :attr:`perf_patterns` will likely be #: deprecated in the future. - perf_patterns = variable(typ.Dict[str, _DeferredExpression], type(None)) + perf_patterns = variable(typ.Dict[str, _DeferredExpression], type(None), + loggable=False) #: The performance variables associated with the test. #: @@ -816,7 +810,7 @@ def pipeline_hooks(cls): #: #: .. versionadded:: 3.8.0 perf_variables = variable(typ.Dict[str, _DeferredPerformanceExpression], - value={}) + value={}, loggable=False) #: List of modules to be loaded before running this test. #: @@ -825,7 +819,7 @@ def pipeline_hooks(cls): #: :type: :class:`List[str]` or :class:`Dict[str, object]` #: :default: ``[]`` modules = variable(typ.List[str], typ.List[typ.Dict[str, object]], - value=[], loggable=True) + value=[]) #: Environment variables to be set before running this test. #: @@ -837,7 +831,7 @@ def pipeline_hooks(cls): #: #: .. versionadded:: 4.0.0 env_vars = variable(typ.Dict[str, str], - typ.Dict[str, object], value={}, loggable=True) + typ.Dict[str, object], value={}) # NOTE: We still keep the original type, just to allow setting this # variable from the command line, because otherwise, ReFrame will not know # how to convert a value to an arbitrary object. @@ -848,7 +842,7 @@ def pipeline_hooks(cls): #: #: .. deprecated:: 4.0.0 #: Please use :attr:`env_vars` instead. - variables = deprecate(variable(alias=env_vars, loggable=True), + variables = deprecate(variable(alias=env_vars), f"the use of 'variables' is deprecated; " f"please use 'env_vars' instead") @@ -880,7 +874,7 @@ def pipeline_hooks(cls): #: The default value is now :class:`None` and it can be set globally #: per partition via the configuration. time_limit = variable(type(None), typ.Duration, value=None, - loggable=True, allow_implicit=True) + allow_implicit=True) #: .. versionadded:: 3.5.1 #: @@ -891,7 +885,7 @@ def pipeline_hooks(cls): #: :type: :class:`str` or :class:`float` or :class:`int` #: :default: :class:`None` build_time_limit = variable(type(None), typ.Duration, value=None, - loggable=True, allow_implicit=True) + allow_implicit=True) #: .. versionadded:: 2.8 #: @@ -964,7 +958,7 @@ def pipeline_hooks(cls): #: A new more powerful syntax was introduced #: that allows also custom job script directive prefixes. extra_resources = variable(typ.Dict[str, typ.Dict[str, object]], - value={}, loggable=True) + value={}) #: .. versionadded:: 3.3 #: @@ -980,7 +974,7 @@ def pipeline_hooks(cls): #: #: :type: boolean #: :default: :class:`True` - build_locally = variable(typ.Bool, value=True, loggable=True) + build_locally = variable(typ.Bool, value=True) #: .. versionadded:: 4.2 #: @@ -1003,12 +997,13 @@ def pipeline_hooks(cls): #: #: :type: :class:`dict` #: :default: ``{}`` - ci_extras = variable(typ.Dict[typ.Str['gitlab'], object], value={}) + ci_extras = variable(typ.Dict[typ.Str['gitlab'], object], value={}, + loggable=False) # Special variables #: Dry-run mode - _rfm_dry_run = variable(typ.Bool, value=False) + _rfm_dry_run = variable(typ.Bool, value=False, loggable=False) def __new__(cls, *args, **kwargs): obj = super().__new__(cls) diff --git a/reframe/core/variables.py b/reframe/core/variables.py index 0939206120..91c54aa84f 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -178,7 +178,8 @@ class MyRequiredTest(HelloTest): 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. + ``check_NAME``, where ``NAME`` is the name of the variable (default + :obj:`True`). :param `kwargs`: keyword arguments to be forwarded to the constructor of the field validator. :returns: A new test variable. @@ -189,6 +190,9 @@ class MyRequiredTest(HelloTest): .. versionadded:: 4.0.0 Alias variable are introduced. + .. versionchanged:: 4.5 + Variables are now loggable by default. + ''' # NOTE: We can't use truly private fields in `__slots__`, because @@ -220,7 +224,7 @@ def __init__(self, *args, **kwargs): else: self._p_default_value = kwargs.pop('value', Undefined) - self._loggable = kwargs.pop('loggable', False) + self._loggable = kwargs.pop('loggable', True) if not issubclass(field_type, fields.Field): raise TypeError( f'field {field_type!r} is not derived from ' diff --git a/unittests/resources/checks/frontend_checks.py b/unittests/resources/checks/frontend_checks.py index 7923672ce0..d0e7205bea 100644 --- a/unittests/resources/checks/frontend_checks.py +++ b/unittests/resources/checks/frontend_checks.py @@ -25,7 +25,7 @@ class BaseFrontendCheck(rfm.RunOnlyRegressionTest): # Add two required variables that may affect the final run report x = variable(int) - xlog = variable(int, loggable=True) + xlog = variable(int) @run_after('setup') def setx(self): diff --git a/unittests/test_logging.py b/unittests/test_logging.py index 7fce8e586b..97009298ab 100644 --- a/unittests/test_logging.py +++ b/unittests/test_logging.py @@ -23,18 +23,16 @@ @pytest.fixture 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) - custom_dict = variable(dict, value={'a': 1, 'b': 2}, loggable=True) + param = parameter(range(3), fmt=lambda x: 10*x) + custom = variable(str, value='hello extras') + custom2 = variable(alias=custom, loggable=False) + custom_list = variable(list, value=['custom', 3.0, ['hello', 'world']]) + custom_dict = variable(dict, value={'a': 1, 'b': 2}) # x is a variable that is loggable, but is left undefined. We want to # make sure that logging does not crash and simply reports is as # undefined - x = variable(str, loggable=True) + x = variable(str) # A bit hacky, but we don't want to run a full test every time test = _FakeCheck(variant_num=1) diff --git a/unittests/test_meta.py b/unittests/test_meta.py index 060df0bfdf..7f868f2151 100644 --- a/unittests/test_meta.py +++ b/unittests/test_meta.py @@ -512,10 +512,10 @@ class Foo(MyMeta): def test_loggable_attrs(): class T(metaclass=meta.RegressionTestMeta): - x = variable(int, value=3, loggable=True) - y = variable(int, loggable=True) # loggable but undefined - z = variable(int) - p = parameter(range(3), loggable=True) + x = variable(int, value=3) + y = variable(int) # loggable but undefined + z = variable(int, loggable=False) + p = parameter(range(3)) @loggable @property @@ -555,6 +555,6 @@ class T(rfm.RegressionTest): def test_deprecated_loggable_attrs(): class T(metaclass=meta.RegressionTestMeta): - x = deprecate(variable(int, value=3, loggable=True), 'deprecated') + x = deprecate(variable(int, value=3), 'deprecated') assert T.loggable_attrs() == [('x', None)] diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 05cc9f8f8c..35ac202bd3 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -1183,7 +1183,6 @@ def foo(self): def test_pinned_hooks(): - @test_util.custom_prefix('unittests/resources/checks') class X(rfm.RunOnlyRegressionTest): @run_before('run', always_last=True) def foo(self): @@ -1207,34 +1206,43 @@ def bar(self): def test_pinned_hooks_multiple_last(): - @test_util.custom_prefix('unittests/resources/checks') class X(rfm.RunOnlyRegressionTest): @run_before('run', always_last=True) - def foo(self): + def hook_a(self): + pass + + @run_before('run') + def hook_b(self): pass class Y(X): @run_before('run', always_last=True) - def bar(self): + def hook_c(self): pass - with pytest.raises(ReframeSyntaxError): - test = Y() + @run_before('run') + def hook_d(self): + pass + + test = Y() + assert test.pipeline_hooks() == { + 'pre_run': [X.hook_b, Y.hook_d, Y.hook_c, X.hook_a] + } def test_pinned_hooks_multiple_last_inherited(): - @test_util.custom_prefix('unittests/resources/checks') class X(rfm.RunOnlyRegressionTest): @run_before('run', always_last=True) def foo(self): pass + class Y(X): @run_before('run', always_last=True) def bar(self): pass - with pytest.raises(ReframeSyntaxError): - test = X() + test = Y() + assert test.pipeline_hooks() == {'pre_run': [Y.bar, X.foo]} def test_disabled_hooks(HelloTest, local_exec_ctx):