From 50ae3b550c4d0beea86619eb5bb82bf287c04984 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sun, 17 Dec 2023 19:19:29 +0100 Subject: [PATCH 1/4] Allow multiple `always_last` --- docs/regression_test_api.rst | 32 ++++++++++++++++++++++++++++++-- reframe/core/builtins.py | 13 ++++++++----- reframe/core/pipeline.py | 25 ++++++++++--------------- unittests/test_pipeline.py | 26 +++++++++++++++----------- 4 files changed, 63 insertions(+), 33 deletions(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 9b839362c7..e0f3d11098 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 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/pipeline.py b/reframe/core/pipeline.py index a6f51ac674..dc090ca3f4 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 diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 05cc9f8f8c..a9629801cd 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,39 @@ 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): - pass + 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): - pass + 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): From ca97f797827afd2192dea276c324b6ff038b7af2 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sun, 17 Dec 2023 19:24:20 +0100 Subject: [PATCH 2/4] Fix PEP8 style issues --- unittests/test_pipeline.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index a9629801cd..35ac202bd3 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -1208,17 +1208,21 @@ def bar(self): def test_pinned_hooks_multiple_last(): class X(rfm.RunOnlyRegressionTest): @run_before('run', always_last=True) - def hook_a(self): pass + def hook_a(self): + pass @run_before('run') - def hook_b(self): pass + def hook_b(self): + pass class Y(X): @run_before('run', always_last=True) - def hook_c(self): pass + def hook_c(self): + pass @run_before('run') - def hook_d(self): pass + def hook_d(self): + pass test = Y() assert test.pipeline_hooks() == { From 18e2787f55ae808bc27bf4036df9145104205f50 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Mon, 18 Dec 2023 22:02:35 +0100 Subject: [PATCH 3/4] Add note about workaround code for hook execution order --- docs/regression_test_api.rst | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index e0f3d11098..04dbec7e83 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -112,7 +112,7 @@ 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 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`: +In the following exampel, :func:`BaseTest.x` will execute before :func:`DerivedTest.y`: .. code-block:: python @@ -174,6 +174,12 @@ the run hooks of :class:`Y` will be executed as follows: 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. From a1568dff97fa450694d86513f77ff64f9c393eb9 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 19 Dec 2023 20:28:52 +0100 Subject: [PATCH 4/4] Fix typo Co-authored-by: Eirini Koutsaniti --- docs/regression_test_api.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 04dbec7e83..4635da5c43 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -112,7 +112,7 @@ 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 normally executed in reverse MRO order, i.e., the hooks of the least specialized class will be executed first. -In the following exampel, :func:`BaseTest.x` will execute before :func:`DerivedTest.y`: +In the following example, :func:`BaseTest.x` will execute before :func:`DerivedTest.y`: .. code-block:: python