From a28348e34430c79d161e9a570e8bee49d1238d7e Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 19 Nov 2021 00:15:12 +0100 Subject: [PATCH 1/3] Fix hook execution order Hooks are now executed in reverse MRO order. --- reframe/core/meta.py | 18 ++++++++------ reframe/core/pipeline.py | 6 ++++- unittests/test_pipeline.py | 51 +++++++++++++++++++++++++++++++++++++- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index ecc2b02f4f..87471dbdf7 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -120,7 +120,7 @@ def __setitem__(self, key, value): # Register the hooks - if a value does not meet the conditions # it will be simply ignored - self['_rfm_hook_registry'].add(value) + self['_rfm_local_hook_registry'].add(value) def __getitem__(self, key): '''Expose and control access to the local namespaces. @@ -306,6 +306,7 @@ def run_after(stage): namespace['run_after'] = run_after namespace['require_deps'] = hooks.require_deps namespace['_rfm_hook_registry'] = hooks.HookRegistry() + namespace['_rfm_local_hook_registry'] = hooks.HookRegistry() # Machinery to add a sanity function def sanity_function(fn): @@ -394,7 +395,7 @@ def __init__(cls, name, bases, namespace, **kwargs): cls._rfm_dir.update(base._rfm_dir) used_attribute_names = set(cls._rfm_dir).union( - {h.__name__ for h in cls._rfm_hook_registry} + {h.__name__ for h in cls._rfm_local_hook_registry} ) # Build the different global class namespaces @@ -409,11 +410,14 @@ def __init__(cls, name, bases, namespace, **kwargs): # Update used names set with the local __dict__ cls._rfm_dir.update(cls.__dict__) - # Update the hook registry with the bases - for base in cls._rfm_bases: - cls._rfm_hook_registry.update( - base._rfm_hook_registry, denied_hooks=namespace - ) + # Populate the global hook registry with the hook registries of the + # parent classes in MRO order + cls._rfm_hook_registry.update(cls._rfm_local_hook_registry) + for c in cls.mro()[1:]: + if hasattr(c, '_rfm_local_hook_registry'): + cls._rfm_hook_registry.update( + c._rfm_local_hook_registry, denied_hooks=namespace + ) # Search the bases if no local sanity functions exist. if '_rfm_sanity' not in namespace: diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index f29f2161b6..acd85fafb6 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -197,11 +197,15 @@ def disable_hook(self, hook_name): @classmethod def pipeline_hooks(cls): + # The hook registry stores hooks in MRO order, but we want to attach + # the hooks in reverse order so that the more specialized hooks (those + # at the beginning of the MRO list) will be executed last + ret = {} for hook in cls._rfm_hook_registry: for stage in hook.stages: try: - ret[stage].append(hook.fn) + ret[stage].insert(0, hook.fn) except KeyError: ret[stage] = [hook.fn] diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index f483ecad5c..63ac7c37b9 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -761,11 +761,60 @@ class MyTest(DerivedTest): assert test.var == 2 assert test.foo == 1 assert test.pipeline_hooks() == { - 'post_setup': [DerivedTest.z, BaseTest.x], + 'post_setup': [BaseTest.x, DerivedTest.z], 'pre_run': [C.y], } +@pytest.fixture +def weird_mro_test(HelloTest): + # This returns a class with non-obvious MRO resolution. + # + # See example in https://www.python.org/download/releases/2.3/mro/ + # + # The MRO of A is ABECDFO, which means that E is more specialized than C! + class X(rfm.RegressionMixin): + pass + + class D(X): + @run_after('setup') + def d(self): + pass + + class E(X): + @run_after('setup') + def e(self): + pass + + class F(X): + @run_after('setup') + def f(self): + pass + + class C(D, F): + @run_after('setup') + def c(self): + pass + + class B(E, D): + @run_after('setup') + def b(self): + pass + + class A(B, C, HelloTest): + @run_after('setup') + def a(self): + pass + + return A + + +def test_inherited_hooks_order(weird_mro_test, local_exec_ctx): + t = weird_mro_test() + hook_order = [fn.__name__ for fn in t.pipeline_hooks()['post_setup']] + assert hook_order == ['f', 'd', 'c', 'e', 'b', 'a'] + + def test_inherited_hooks_from_instantiated_tests(HelloTest, local_exec_ctx): @test_util.custom_prefix('unittests/resources/checks') class T0(HelloTest): From aa1f10dc56130e4f9d4b6a78686dfeabe58ef94d Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 19 Nov 2021 11:42:28 +0100 Subject: [PATCH 2/3] Update documentation --- docs/regression_test_api.rst | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 4f1d4b29df..d0f686e771 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -346,7 +346,7 @@ In essence, these builtins exert control over the test creation, and they allow @rfm.simple_test class TestC(rfm.RegressionTest): # Parameterize TestC for each ParamFix variant - f = fixture(ParamFix, action='fork') + f = fixture(ParamFix, action='fork') ... @run_after('setup') @@ -546,7 +546,7 @@ Pipeline hooks attached to multiple stages will be executed on each pipeline sta Pipeline stages with multiple hooks attached will execute these hooks in the order in which they were attached to the given pipeline stage. A derived class will inherit all the pipeline hooks defined in its bases, except for those whose hook function is overridden by the derived class. A function that overrides a pipeline hook from any of the base classes will not be a pipeline hook unless the overriding function is explicitly reattached to any pipeline stage. -In the event of a name clash arising from multiple inheritance, the inherited pipeline hook will be chosen following Python's `MRO `_. +In the event of a name clash arising from multiple inheritance, the inherited pipeline hook will be chosen following Python's `MRO `__. A function may be attached to any of the following stages (listed in order of execution): ``init``, ``setup``, ``compile``, ``run``, ``sanity``, ``performance`` and ``cleanup``. The ``init`` stage refers to the test's instantiation and it runs before entering the execution pipeline. @@ -556,6 +556,22 @@ 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. +In the following example, :func:`BaseTest.x` will execute before :func:`DerivedTest.y`: + +.. code:: python + + class BaseTest(rfm.RegressionTest): + @run_after('setup') + def x(self): + '''Hook x''' + + class DerivedTest(BaseTeset): + @run_after('setup') + def y(self): + '''Hook y''' + + .. note:: Pipeline hooks do not execute in the test's stage directory. However, the test's :attr:`~reframe.core.pipeline.RegressionTest.stagedir` can be accessed by explicitly changing the working directory from within the hook function itself (see the :class:`~reframe.utility.osext.change_dir` utility for further details): @@ -577,6 +593,16 @@ The framework will then continue with other activities and it will execute the p Declaring pipeline hooks using the same name functions from the :py:mod:`reframe` or :py:mod:`reframe.core.decorators` modules is now deprecated. You should use the built-in functions described in this section instead. +.. warning:: + .. versionchanged:: 3.9.2 + + Execution of pipeline hooks until this version was implementation-defined. + In practice, hooks of a derived class were executed before those of its parents. + + This version defines the execution order of hooks, which now follows a strict reverse MRO order, so that parent hooks will execute before those of derived classes. + Tests that relied on the execution order of hooks might break with this change. + + .. py:decorator:: RegressionMixin.run_before(stage) Decorator for attaching a function to a given pipeline stage. From 99ddbc8574b2398dc011431e5015453283e8f5ed Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 19 Nov 2021 11:44:36 +0100 Subject: [PATCH 3/3] Fix typo in unit tests --- unittests/test_pipeline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 63ac7c37b9..d76da6be5a 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -772,7 +772,7 @@ def weird_mro_test(HelloTest): # # See example in https://www.python.org/download/releases/2.3/mro/ # - # The MRO of A is ABECDFO, which means that E is more specialized than C! + # The MRO of A is ABECDFX, which means that E is more specialized than C! class X(rfm.RegressionMixin): pass