From 5fdf0ca425f90588499ffa8fd927ed2736bd9cc4 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sat, 13 Mar 2021 16:06:42 +0100 Subject: [PATCH 01/16] Add post-init hook --- docs/regression_test_api.rst | 18 +++++++++++++++--- reframe/core/decorators.py | 28 ++++++++++++++++++++++++---- reframe/core/pipeline.py | 1 + 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 46f43f6bfa..145ec7bd38 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -29,6 +29,18 @@ Regression Test Class Decorators Pipeline Hooks -------------- +.. versionadded:: 2.20 + + +Pipeline hooks is an easy way to perform operations while the test traverses the execution pipeline. +You can attach arbitrary functions to run before or after any pipeline stage, which are called *pipeline hooks*. +Multiple hooks can be attached before or after the same pipeline stage, in which case the order of execution will match the order in which the functions are defined in the class body of the test. +A single hook can also be applied to multiple stages and it will be executed multiple times. +All pipeline hooks of a test class are inherited by its subclasses. +Subclasses may override a pipeline hook of their parents by redefining the hook function and re-attaching it at the same pipeline stage. +There are seven pipeline stages where you can attach test methods: ``init``, ``setup``, ``compile``, ``run``, ``sanity``, ``performance`` and ``cleanup``. + + .. autodecorator:: reframe.core.decorators.run_after(stage) .. autodecorator:: reframe.core.decorators.run_before(stage) @@ -67,7 +79,7 @@ In essence, these builtins exert control over the test creation, and they allow else: do_other() - One of the most powerful features about these built-in functions is that they store their input information at the class level. + One of the most powerful features about these built-in functions is that they store their input information at the class level. However, a parameter may only be accessed from the class instance and accessing it directly from the class body is disallowed. With this approach, extending or specializing an existing parametrized regression test becomes straightforward, since the test attribute additions and modifications made through built-in functions in the parent class are automatically inherited by the child test. For instance, continuing with the example above, one could override the :func:`__init__` method in the :class:`Foo` regression test as follows: @@ -83,7 +95,7 @@ In essence, these builtins exert control over the test creation, and they allow Note that this built-in parameter function provides an alternative method to parameterize a test to :func:`reframe.core.decorators.parameterized_test`, and the use of both approaches in the same test is currently disallowed. The two main advantages of the built-in :func:`parameter` over the decorated approach reside in the parameter inheritance across classes and the handling of large parameter sets. - As shown in the example above, the parameters declared with the built-in :func:`parameter` are automatically carried over into derived tests through class inheritance, whereas tests using the decorated approach would have to redefine the parameters on every test. + As shown in the example above, the parameters declared with the built-in :func:`parameter` are automatically carried over into derived tests through class inheritance, whereas tests using the decorated approach would have to redefine the parameters on every test. Similarly, parameters declared through the built-in :func:`parameter` are regarded as fully independent from each other and ReFrame will automatically generate as many tests as available parameter combinations. This is a major advantage over the decorated approach, where one would have to manually expand the parameter combinations. This is illustrated in the example below, consisting of a case with two parameters, each having two possible values. @@ -118,7 +130,7 @@ In essence, these builtins exert control over the test creation, and they allow Inserts a new regression test variable. Declaring a test variable through the :func:`variable` built-in allows for a more robust test implementation than if the variables were just defined as regular test attributes (e.g. ``self.a = 10``). - Using variables declared through the :func:`variable` built-in guarantees that these regression test variables will not be redeclared by any child class, while also ensuring that any values that may be assigned to such variables comply with its original declaration. + Using variables declared through the :func:`variable` built-in guarantees that these regression test variables will not be redeclared by any child class, while also ensuring that any values that may be assigned to such variables comply with its original declaration. In essence, by using test variables, the user removes any potential test errors that might be caused by accidentally overriding a class attribute. See the example below. diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 72daa9a2d4..2e019c8985 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -221,7 +221,6 @@ def run_before(stage): The ``stage`` argument can be any of ``'setup'``, ``'compile'``, ``'run'``, ``'sanity'``, ``'performance'`` or ``'cleanup'``. - .. versionadded:: 2.20 ''' return _runx('pre_' + stage) @@ -229,10 +228,31 @@ def run_before(stage): def run_after(stage): '''Decorator for attaching a test method to a pipeline stage. - This is completely analogous to the - :py:attr:`reframe.core.decorators.run_before`. + This is analogous to the :py:attr:`~reframe.core.decorators.run_before`, + except that ``'init'`` can also be used as the ``stage`` argument. In this + case, the hook will execute right after the test is initialized. A + post-init hook is equivalent to defining the :func:`__init__` function in + the test. However, if :func:`__init__` is defined in the test, any + post-init hooks will be ignored. All the other properties of pipeline + hooks apply equally here. The following code + + .. code-block:: python + + @rfm.run_after('init') + def foo(self): + self.x = 1 + + + is equivalent to + + .. code-block:: python + + def __init__(self): + self.x = 1 + + .. versionchanged:: 3.5.1 + Add the ability to define post-init hooks in tests. - .. versionadded:: 2.20 ''' return _runx('post_' + stage) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 309311a95d..487684d7d2 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -786,6 +786,7 @@ def __new__(cls, *args, _rfm_use_params=False, **kwargs): obj._rfm_init(name, prefix) return obj + @_run_hooks('post_init') def __init__(self): pass From 7d17ba344077e348e8da4d144319b8e9864aeb6a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sun, 14 Mar 2021 12:32:12 +0100 Subject: [PATCH 02/16] Allow running pre-init hooks, but only if set by the framework --- reframe/core/decorators.py | 3 +++ reframe/core/pipeline.py | 8 ++++++-- unittests/test_pipeline.py | 25 +++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 2e019c8985..ccd9b10c2d 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -222,6 +222,9 @@ def run_before(stage): ``'run'``, ``'sanity'``, ``'performance'`` or ``'cleanup'``. ''' + if stage == 'init': + raise ValueError('pre-init hooks are not allowed') + return _runx('pre_' + stage) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 487684d7d2..f6e4ddc34c 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -82,7 +82,11 @@ def _run_hooks(name=None): def _deco(func): def hooks(obj, kind): if name is None: - hook_name = kind + func.__name__ + fn_name = func.__name__ + if fn_name == '__init__': + fn_name = 'init' + + hook_name = kind + fn_name elif name is not None and name.startswith(kind): hook_name = name else: @@ -786,7 +790,7 @@ def __new__(cls, *args, _rfm_use_params=False, **kwargs): obj._rfm_init(name, prefix) return obj - @_run_hooks('post_init') + @_run_hooks() def __init__(self): pass diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 4334bb9096..8a208964d3 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -494,6 +494,31 @@ def set_resources(self): assert expected_job_options == set(test.job.options) +def test_pre_init_hook(local_exec_ctx): + with pytest.raises(ValueError): + @fixtures.custom_prefix('unittests/resources/checks') + class MyTest(rfm.RunOnlyRegressionTest): + @rfm.run_before('init') + def prepare(self): + self.x = 1 + + +def test_post_init_hook(local_exec_ctx): + @fixtures.custom_prefix('unittests/resources/checks') + class MyTest(rfm.RunOnlyRegressionTest): + valid_systems = ['*'] + valid_prog_environs = ['*'] + executable = 'echo' + executable_opts = ['hello'] + + @rfm.run_after('init') + def prepare(self): + self.sanity_patterns = sn.assert_found(r'hello', self.stdout) + + test = MyTest() + _run(test, *local_exec_ctx) + + def test_setup_hooks(HelloTest, local_exec_ctx): @fixtures.custom_prefix('unittests/resources/checks') class MyTest(HelloTest): From e39da2fb23fb4437c20587961f8788d4670abc52 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 16 Mar 2021 19:23:44 +0100 Subject: [PATCH 03/16] Expand unit tests --- reframe/core/meta.py | 1 + reframe/core/pipeline.py | 1 - unittests/test_pipeline.py | 19 ++++++++++--------- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 16058cd8a6..b494d36db2 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -18,6 +18,7 @@ class RegressionTestMeta(type): class MetaNamespace(namespaces.LocalNamespace): '''Custom namespace to control the cls attribute assignment.''' + def __setitem__(self, key, value): if isinstance(value, variables.VarDirective): # Insert the attribute in the variable namespace diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index f6e4ddc34c..8e6f854693 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -790,7 +790,6 @@ def __new__(cls, *args, _rfm_use_params=False, **kwargs): obj._rfm_init(name, prefix) return obj - @_run_hooks() def __init__(self): pass diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 8a208964d3..f95157e828 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -504,19 +504,20 @@ def prepare(self): def test_post_init_hook(local_exec_ctx): - @fixtures.custom_prefix('unittests/resources/checks') - class MyTest(rfm.RunOnlyRegressionTest): - valid_systems = ['*'] - valid_prog_environs = ['*'] - executable = 'echo' - executable_opts = ['hello'] + class _MyTest(rfm.RunOnlyRegressionTest): + x = variable(int, value=0) + y = variable(int, value=0) + + def __init__(self): + self.x = 1 @rfm.run_after('init') def prepare(self): - self.sanity_patterns = sn.assert_found(r'hello', self.stdout) + self.y = 1 - test = MyTest() - _run(test, *local_exec_ctx) + test = _MyTest() + assert test.x == 1 + assert test.y == 1 def test_setup_hooks(HelloTest, local_exec_ctx): From f1047f9a541b7e4e9e2fda268700cbbf01c66915 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 25 Mar 2021 23:47:52 +0100 Subject: [PATCH 04/16] Reimplement pipeline hooks Pipeline hooks are now implemented in the metaclass. The logic is very similar to that of the variables and parameters. As the class hierarchy is built, a `HookRegistry` is maintained containing all the hooks. When the test object is created for the first time, all of its pipeline methods are wrapped with the corresponding pipeline hooks. Moving the implementation to the metaclasses brings a set of advantages: - Inheritance is handled by Python and there is no need to manually ascend the class hierarchy when the hook is applied. - For storing the hooks of each phase, we use an ordered set and we make sure that firstly the hooks of the class that is being currently created are inserted and then those of its parents. This way we ensure that hooks are properly overriden. - By using metaclasses, hooks are more consistent and are automatically applied correctly to any method overrides. --- reframe/core/hooks.py | 109 +++++++++++++++++++++++++++++++++++++ reframe/core/meta.py | 23 +++++--- reframe/core/pipeline.py | 91 ++++++------------------------- reframe/frontend/cli.py | 2 +- unittests/test_pipeline.py | 24 +++++--- 5 files changed, 161 insertions(+), 88 deletions(-) create mode 100644 reframe/core/hooks.py diff --git a/reframe/core/hooks.py b/reframe/core/hooks.py new file mode 100644 index 0000000000..08f2545868 --- /dev/null +++ b/reframe/core/hooks.py @@ -0,0 +1,109 @@ +# Copyright 2016-2021 Swiss National Supercomputing Centre (CSCS/ETH Zurich) +# ReFrame Project Developers. See the top-level LICENSE file for details. +# +# SPDX-License-Identifier: BSD-3-Clause + +import functools + +import reframe.utility as util + + +def _attach_hooks(hooks, name=None): + def _deco(func): + def select_hooks(obj, kind): + if name is None: + fn_name = func.__name__ + if fn_name == '__init__': + fn_name = 'init' + + hook_kind = kind + fn_name + elif name is not None and name.startswith(kind): + hook_kind = name + else: + # Just any name that does not exist + hook_kind = 'xxx' + + if hook_kind not in hooks: + return [] + + return [h for h in hooks[hook_kind] + if h.name not in obj._disabled_hooks] + + @functools.wraps(func) + def _fn(obj, *args, **kwargs): + for h in select_hooks(obj, 'pre_'): + h(obj) + + func(obj, *args, **kwargs) + for h in select_hooks(obj, 'post_'): + h(obj) + + return _fn + + return _deco + + +class Hook: + def __init__(self, fn): + self.__fn = fn + + @property + def fn(self): + return self.__fn + + @property + def name(self): + return self.__fn.__name__ + + def __hash__(self): + return hash(self.name) + + def __eq__(self, other): + if not isinstance(other, type(self)): + return NotImplemented + + return self.name == other.name + + def __call__(self, *args, **kwargs): + return self.__fn(*args, **kwargs) + + def __repr__(self): + return repr(self.__fn) + + +class HookRegistry: + '''Global hook registry.''' + + def __init__(self, hooks=None): + self.__hooks = {} + if hooks is not None: + self.update(hooks) + + def __getitem__(self, key): + return self.__hooks[key] + + def __setitem__(self, key, name): + self.__hooks[key] = name + + def __getattr__(self, name): + return getattr(self.__hooks, name) + + def update(self, hooks): + for phase, hks in hooks.items(): + self.__hooks.setdefault(phase, util.OrderedSet()) + for h in hks: + self.__hooks[phase].add(h) + + def apply(self, obj): + cls = type(obj) + cls.__init__ = _attach_hooks(self.__hooks)(cls.__init__) + cls.setup = _attach_hooks(self.__hooks)(cls.setup) + cls.compile = _attach_hooks(self.__hooks, 'pre_compile')(cls.compile) + cls.compile_wait = _attach_hooks(self.__hooks, 'post_compile')( + cls.compile_wait + ) + cls.run = _attach_hooks(self.__hooks, 'pre_run')(cls.run) + cls.run_wait = _attach_hooks(self.__hooks, 'post_run')(cls.run_wait) + cls.sanity = _attach_hooks(self.__hooks)(cls.sanity) + cls.performance = _attach_hooks(self.__hooks)(cls.performance) + cls.cleanup = _attach_hooks(self.__hooks)(cls.cleanup) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 7cc43f6944..015abb3846 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -8,11 +8,13 @@ # -from reframe.core.exceptions import ReframeSyntaxError import reframe.core.namespaces as namespaces import reframe.core.parameters as parameters import reframe.core.variables as variables +from reframe.core.exceptions import ReframeSyntaxError +from reframe.core.hooks import Hook, HookRegistry + class RegressionTestMeta(type): @@ -143,27 +145,34 @@ def __init__(cls, name, bases, namespace, **kwargs): # Set up the hooks for the pipeline stages based on the _rfm_attach # attribute; all dependencies will be resolved first in the post-setup # phase if not assigned elsewhere - hooks = {} + local_hooks = {} fn_with_deps = [] for v in namespace.values(): if hasattr(v, '_rfm_attach'): for phase in v._rfm_attach: try: - hooks[phase].append(v) + local_hooks[phase].append(Hook(v)) except KeyError: - hooks[phase] = [v] + local_hooks[phase] = [Hook(v)] try: if v._rfm_resolve_deps: - fn_with_deps.append(v) + fn_with_deps.append(Hook(v)) except AttributeError: pass if fn_with_deps: - hooks['post_setup'] = fn_with_deps + hooks.get('post_setup', []) + local_hooks['post_setup'] = ( + fn_with_deps + local_hooks.get('post_setup', []) + ) + + hooks = HookRegistry(local_hooks) + for b in bases: + if hasattr(b, '_rfm_pipeline_hooks'): + hooks.update(getattr(b, '_rfm_pipeline_hooks')) + hooks.update(local_hooks) cls._rfm_pipeline_hooks = hooks - cls._rfm_disabled_hooks = set() cls._final_methods = {v.__name__ for v in namespace.values() if hasattr(v, '_rfm_final')} diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 48e1cf7301..45b8a1ad3f 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -78,58 +78,6 @@ DEPEND_FULLY = 3 -def _run_hooks(name=None): - def _deco(func): - def hooks(obj, kind): - if name is None: - fn_name = func.__name__ - if fn_name == '__init__': - fn_name = 'init' - - hook_name = kind + fn_name - elif name is not None and name.startswith(kind): - hook_name = name - else: - # Just any name that does not exist - hook_name = 'xxx' - - func_names = set() - disabled_hooks = set() - func_list = [] - for cls in type(obj).mro(): - if hasattr(cls, '_rfm_disabled_hooks'): - disabled_hooks |= cls._rfm_disabled_hooks - - try: - funcs = cls._rfm_pipeline_hooks.get(hook_name, []) - if any(fn.__name__ in func_names for fn in funcs): - # hook has been overriden - continue - - func_names |= {fn.__name__ for fn in funcs} - func_list += funcs - except AttributeError: - pass - - # Remove the disabled hooks before returning - return [fn for fn in func_list - if fn.__name__ not in disabled_hooks] - - '''Run the hooks before and after func.''' - @functools.wraps(func) - def _fn(obj, *args, **kwargs): - for h in hooks(obj, 'pre_'): - h(obj) - - func(obj, *args, **kwargs) - for h in hooks(obj, 'post_'): - h(obj) - - return _fn - - return _deco - - def final(fn): fn._rfm_final = True @@ -183,24 +131,22 @@ class RegressionTest(RegressionMixin, jsonext.JSONSerializable): ''' - @classmethod - def disable_hook(cls, hook_name): + def disable_hook(self, hook_name): '''Disable pipeline hook by name. :arg hook_name: The function name of the hook to be disabled. :meta private: ''' - cls._rfm_disabled_hooks.add(hook_name) + self._disabled_hooks.add(hook_name) @classmethod def pipeline_hooks(cls): ret = {} - for c in cls.mro(): - if hasattr(c, '_rfm_pipeline_hooks'): - for kind, hook in c._rfm_pipeline_hooks.items(): - ret.setdefault(kind, []) - ret[kind] += hook + for phase, hooks in cls._rfm_pipeline_hooks.items(): + ret[phase] = [] + for h in hooks: + ret[phase].append(h.fn) return ret @@ -803,7 +749,13 @@ def __new__(cls, *args, _rfm_use_params=False, **kwargs): os.path.dirname(inspect.getfile(cls)) ) - obj._rfm_init(name, prefix) + # Apply the hooks only once (not when deep-copying) + if not hasattr(cls, '_rfm_first_init'): + cls._rfm_pipeline_hooks.apply(obj) + cls._rfm_first_init = True + + # Initialize the test + obj.__rfm_init__(name, prefix) return obj def __init__(self): @@ -828,7 +780,7 @@ def __init_subclass__(cls, *, special=False, pin_prefix=False, **kwargs): os.path.dirname(inspect.getfile(cls)) ) - def _rfm_init(self, name=None, prefix=None): + def __rfm_init__(self, name=None, prefix=None): if name is not None: self.name = name @@ -887,7 +839,11 @@ def _rfm_init(self, name=None, prefix=None): # Just an empty environment self._cdt_environ = env.Environment('__rfm_cdt_environ') + # Disabled hooks + self._disabled_hooks = set() + # Export read-only views to interesting fields + @property def current_environ(self): '''The programming environment that the regression test is currently @@ -1116,7 +1072,6 @@ def _setup_job(self, name, force_local=False, **job_opts): def _setup_perf_logging(self): self._perf_logger = logging.getperflogger(self) - @_run_hooks() @final def setup(self, partition, environ, **job_opts): '''The setup phase of the regression test pipeline. @@ -1166,7 +1121,6 @@ def _clone_to_stagedir(self, url): self.logger.debug(f'Cloning URL {url} into stage directory') osext.git_clone(self.sourcesdir, self._stagedir) - @_run_hooks('pre_compile') @final def compile(self): '''The compilation phase of the regression test pipeline. @@ -1273,7 +1227,6 @@ def compile(self): self._build_job.submit() - @_run_hooks('post_compile') @final def compile_wait(self): '''Wait for compilation phase to finish. @@ -1303,7 +1256,6 @@ def compile_wait(self): self.build_system.post_build(self._build_job) - @_run_hooks('pre_run') @final def run(self): '''The run phase of the regression test pipeline. @@ -1454,7 +1406,6 @@ def poll(self): 'please use run_complete() instead') return self.run_complete() - @_run_hooks('post_run') @final def run_wait(self): '''Wait for the run phase of this test to finish. @@ -1485,12 +1436,10 @@ def wait(self): 'please use run_wait() instead') self.run_wait() - @_run_hooks() @final def sanity(self): self.check_sanity() - @_run_hooks() @final def performance(self): try: @@ -1658,7 +1607,6 @@ def _copy_to_outputdir(self): else: shutil.copy2(f, self.outputdir) - @_run_hooks() @final def cleanup(self, remove_files=False): '''The cleanup phase of the regression test pipeline. @@ -1892,7 +1840,6 @@ class RunOnlyRegressionTest(RegressionTest, special=True): module. ''' - @_run_hooks() def setup(self, partition, environ, **job_opts): '''The setup stage of the regression test pipeline. @@ -1918,7 +1865,6 @@ def compile_wait(self): This is a no-op for this type of test. ''' - @_run_hooks('pre_run') def run(self): '''The run phase of the regression test pipeline. @@ -1948,7 +1894,6 @@ class CompileOnlyRegressionTest(RegressionTest, special=True): module. ''' - @_run_hooks() def setup(self, partition, environ, **job_opts): '''The setup stage of the regression test pipeline. diff --git a/reframe/frontend/cli.py b/reframe/frontend/cli.py index c6a14459bd..e107ec5c67 100644 --- a/reframe/frontend/cli.py +++ b/reframe/frontend/cli.py @@ -796,7 +796,7 @@ def _case_failed(t): # Disable hooks for tc in testcases: for h in options.hooks: - type(tc.check).disable_hook(h) + tc.check.disable_hook(h) # Act on checks if options.list or options.list_detailed: diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index f95157e828..3a3f012853 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -504,20 +504,30 @@ def prepare(self): def test_post_init_hook(local_exec_ctx): - class _MyTest(rfm.RunOnlyRegressionTest): + class _T0(rfm.RunOnlyRegressionTest): x = variable(int, value=0) - y = variable(int, value=0) + y = variable(int, value=1) def __init__(self): self.x = 1 @rfm.run_after('init') def prepare(self): - self.y = 1 + self.y += 1 - test = _MyTest() - assert test.x == 1 - assert test.y == 1 + class _T1(_T0): + def __init__(self): + super().__init__() + self.z = 3 + + t0 = _T0() + assert t0.x == 1 + assert t0.y == 2 + + t1 = _T1() + assert t1.x == 1 + assert t1.y == 2 + assert t1.z == 3 def test_setup_hooks(HelloTest, local_exec_ctx): @@ -737,7 +747,7 @@ def x(self): self.var += 5 test = MyTest() - MyTest.disable_hook('y') + test.disable_hook('y') _run(test, *local_exec_ctx) assert test.var == 5 assert test.foo == 0 From 1fbeb38d4baec2ac76645a5e8ba6a7f74c62218a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 26 Mar 2021 22:25:38 +0100 Subject: [PATCH 05/16] Update docstring for post-init hooks Co-authored-by: Javier Otero <71280927+jjotero@users.noreply.github.com> --- reframe/core/decorators.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index c9091e170d..c18511d01b 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -238,11 +238,11 @@ def run_after(stage): This is analogous to the :py:attr:`~reframe.core.decorators.run_before`, except that ``'init'`` can also be used as the ``stage`` argument. In this - case, the hook will execute right after the test is initialized. A - post-init hook is equivalent to defining the :func:`__init__` function in - the test. However, if :func:`__init__` is defined in the test, any - post-init hooks will be ignored. All the other properties of pipeline - hooks apply equally here. The following code + case, the hook will execute right after the test is initialized (i.e. after the + :func:`__init__` method is called), before entering the test's pipeline. + In essence, a post-init hook is equivalent to defining additional + :func:`__init__` functions in the test. All the other properties of + pipeline hooks apply equally here. The following code .. code-block:: python From 8ccc22e0b6ebbd966689049325687d754e680ee3 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 26 Mar 2021 23:36:46 +0100 Subject: [PATCH 06/16] Address PR comments (I) --- reframe/core/decorators.py | 10 +++++----- reframe/core/hooks.py | 17 ++++++++--------- reframe/core/meta.py | 6 +++--- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index c18511d01b..a0cb5c81c8 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -238,11 +238,11 @@ def run_after(stage): This is analogous to the :py:attr:`~reframe.core.decorators.run_before`, except that ``'init'`` can also be used as the ``stage`` argument. In this - case, the hook will execute right after the test is initialized (i.e. after the - :func:`__init__` method is called), before entering the test's pipeline. - In essence, a post-init hook is equivalent to defining additional - :func:`__init__` functions in the test. All the other properties of - pipeline hooks apply equally here. The following code + case, the hook will execute right after the test is initialized (i.e. + after the :func:`__init__` method is called), before entering the test's + pipeline. In essence, a post-init hook is equivalent to defining + additional :func:`__init__` functions in the test. All the other + properties of pipeline hooks apply equally here. The following code .. code-block:: python diff --git a/reframe/core/hooks.py b/reframe/core/hooks.py index 08f2545868..b4a8f6d5ee 100644 --- a/reframe/core/hooks.py +++ b/reframe/core/hooks.py @@ -11,22 +11,21 @@ def _attach_hooks(hooks, name=None): def _deco(func): def select_hooks(obj, kind): - if name is None: + phase = name + if phase is None: fn_name = func.__name__ if fn_name == '__init__': fn_name = 'init' - hook_kind = kind + fn_name - elif name is not None and name.startswith(kind): - hook_kind = name - else: - # Just any name that does not exist - hook_kind = 'xxx' + phase = kind + fn_name + elif phase is not None and not phase.startswith(kind): + # Just any phase that does not exist + phase = 'xxx' - if hook_kind not in hooks: + if phase not in hooks: return [] - return [h for h in hooks[hook_kind] + return [h for h in hooks[phase] if h.name not in obj._disabled_hooks] @functools.wraps(func) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 015abb3846..bd749a1d6f 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -8,6 +8,8 @@ # +import contextlib + import reframe.core.namespaces as namespaces import reframe.core.parameters as parameters import reframe.core.variables as variables @@ -155,11 +157,9 @@ def __init__(cls, name, bases, namespace, **kwargs): except KeyError: local_hooks[phase] = [Hook(v)] - try: + with contextlib.suppress(AttributeError): if v._rfm_resolve_deps: fn_with_deps.append(Hook(v)) - except AttributeError: - pass if fn_with_deps: local_hooks['post_setup'] = ( From 6390cb1a4cad03e5adb529f9055006d14b42ea44 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 30 Mar 2021 00:01:01 +0200 Subject: [PATCH 07/16] Reimplement the pipeline hook mechanism Pipeline hooks are no more applied to the pipeline functions with the decorator. This has the following advantages: a. Hooks will be automatically applied to any pipeline function that is overriden by a subclass. b. The hooks will wrap the leaf function and not the call to `super().pipeline_fn()` c. The hooks will be applied only once in all cases even if a test and its child classes are instantiated multiple times. --- docs/regression_test_api.rst | 5 +++ reframe/core/decorators.py | 2 +- reframe/core/hooks.py | 81 ++++++++++++++++++++++++++---------- reframe/core/meta.py | 24 +---------- reframe/core/pipeline.py | 51 ++++++++++++++++++++--- reframe/utility/__init__.py | 2 +- unittests/test_ci.py | 9 +++- unittests/test_pipeline.py | 29 +++++++++++++ 8 files changed, 151 insertions(+), 52 deletions(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index d245079dc6..63d870244e 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -39,7 +39,12 @@ A single hook can also be applied to multiple stages and it will be executed mul All pipeline hooks of a test class are inherited by its subclasses. Subclasses may override a pipeline hook of their parents by redefining the hook function and re-attaching it at the same pipeline stage. There are seven pipeline stages where you can attach test methods: ``init``, ``setup``, ``compile``, ``run``, ``sanity``, ``performance`` and ``cleanup``. +The ``init`` stage is not a real pipeline stage, but it refers to the test initialization. +Hooks attached to any stage will run exactly before or after this stage executes. +So although a "post-init" and a "pre-setup" hook will both run *after* a test has been initialized and *before* the test goes through the first pipeline stage, they will execute in different times: +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. .. autodecorator:: reframe.core.decorators.run_after(stage) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index a0cb5c81c8..ebe807f40d 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -258,7 +258,7 @@ def foo(self): def __init__(self): self.x = 1 - .. versionchanged:: 3.5.1 + .. versionchanged:: 3.5.2 Add the ability to define post-init hooks in tests. ''' diff --git a/reframe/core/hooks.py b/reframe/core/hooks.py index b4a8f6d5ee..77977d76b3 100644 --- a/reframe/core/hooks.py +++ b/reframe/core/hooks.py @@ -3,12 +3,22 @@ # # SPDX-License-Identifier: BSD-3-Clause +import contextlib import functools import reframe.utility as util -def _attach_hooks(hooks, name=None): +def attach_hooks(hooks, name=None): + '''Attach pipeline hooks to phase ``name''. + + This function returns a decorator for pipeline functions that will run the + registered hooks before and after the function. + + If ``name'' is :class:`None`, both pre- and post-hooks will run, otherwise + only the hooks of the phase ``name'' will be executed. + ''' + def _deco(func): def select_hooks(obj, kind): phase = name @@ -26,7 +36,9 @@ def select_hooks(obj, kind): return [] return [h for h in hooks[phase] - if h.name not in obj._disabled_hooks] + if h.__name__ not in obj._disabled_hooks] + + func.__rfm_pipeline_fn__ = func @functools.wraps(func) def _fn(obj, *args, **kwargs): @@ -43,25 +55,30 @@ def _fn(obj, *args, **kwargs): class Hook: + '''A pipeline hook. + + This is essentially a function wrapper that hashes the functions by name, + since we want hooks to be overriden by name in subclasses. + ''' + def __init__(self, fn): self.__fn = fn + def __getattr__(self, attr): + return getattr(self.__fn, attr) + @property def fn(self): return self.__fn - @property - def name(self): - return self.__fn.__name__ - def __hash__(self): - return hash(self.name) + return hash(self.__name__) def __eq__(self, other): if not isinstance(other, type(self)): return NotImplemented - return self.name == other.name + return self.__name__ == other.__name__ def __call__(self, *args, **kwargs): return self.__fn(*args, **kwargs) @@ -73,6 +90,36 @@ def __repr__(self): class HookRegistry: '''Global hook registry.''' + @classmethod + def create(cls, namespace): + '''Create a hook registry from a class namespace. + + Hook functions have an `_rfm_attach` attribute that specify the stages + of the pipeline where they must be attached. Dependencies will be + resolved first in the post-setup phase if not assigned elsewhere. + ''' + + local_hooks = {} + fn_with_deps = [] + for v in namespace.values(): + if hasattr(v, '_rfm_attach'): + for phase in v._rfm_attach: + try: + local_hooks[phase].append(Hook(v)) + except KeyError: + local_hooks[phase] = [Hook(v)] + + with contextlib.suppress(AttributeError): + if v._rfm_resolve_deps: + fn_with_deps.append(Hook(v)) + + if fn_with_deps: + local_hooks['post_setup'] = ( + fn_with_deps + local_hooks.get('post_setup', []) + ) + + return cls(local_hooks) + def __init__(self, hooks=None): self.__hooks = {} if hooks is not None: @@ -84,6 +131,9 @@ def __getitem__(self, key): def __setitem__(self, key, name): self.__hooks[key] = name + def __contains__(self, key): + return key in self.__hooks + def __getattr__(self, name): return getattr(self.__hooks, name) @@ -93,16 +143,5 @@ def update(self, hooks): for h in hks: self.__hooks[phase].add(h) - def apply(self, obj): - cls = type(obj) - cls.__init__ = _attach_hooks(self.__hooks)(cls.__init__) - cls.setup = _attach_hooks(self.__hooks)(cls.setup) - cls.compile = _attach_hooks(self.__hooks, 'pre_compile')(cls.compile) - cls.compile_wait = _attach_hooks(self.__hooks, 'post_compile')( - cls.compile_wait - ) - cls.run = _attach_hooks(self.__hooks, 'pre_run')(cls.run) - cls.run_wait = _attach_hooks(self.__hooks, 'post_run')(cls.run_wait) - cls.sanity = _attach_hooks(self.__hooks)(cls.sanity) - cls.performance = _attach_hooks(self.__hooks)(cls.performance) - cls.cleanup = _attach_hooks(self.__hooks)(cls.cleanup) + def __repr__(self): + return repr(self.__hooks) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index bd749a1d6f..9564306a0f 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -147,32 +147,12 @@ def __init__(cls, name, bases, namespace, **kwargs): # Set up the hooks for the pipeline stages based on the _rfm_attach # attribute; all dependencies will be resolved first in the post-setup # phase if not assigned elsewhere - local_hooks = {} - fn_with_deps = [] - for v in namespace.values(): - if hasattr(v, '_rfm_attach'): - for phase in v._rfm_attach: - try: - local_hooks[phase].append(Hook(v)) - except KeyError: - local_hooks[phase] = [Hook(v)] - - with contextlib.suppress(AttributeError): - if v._rfm_resolve_deps: - fn_with_deps.append(Hook(v)) - - if fn_with_deps: - local_hooks['post_setup'] = ( - fn_with_deps + local_hooks.get('post_setup', []) - ) - - hooks = HookRegistry(local_hooks) + hooks = HookRegistry.create(namespace) for b in bases: if hasattr(b, '_rfm_pipeline_hooks'): hooks.update(getattr(b, '_rfm_pipeline_hooks')) - hooks.update(local_hooks) - cls._rfm_pipeline_hooks = hooks + cls._rfm_pipeline_hooks = hooks # HookRegistry(local_hooks) cls._final_methods = {v.__name__ for v in namespace.values() if hasattr(v, '_rfm_final')} diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 45b8a1ad3f..42574e72bc 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -24,6 +24,7 @@ import reframe.core.environments as env import reframe.core.fields as fields +import reframe.core.hooks as hooks import reframe.core.logging as logging import reframe.core.runtime as rt import reframe.utility as util @@ -717,7 +718,15 @@ def pipeline_hooks(cls): #: :type: boolean : :default: :class:`True` build_locally = variable(bool, value=True) - def __new__(cls, *args, _rfm_use_params=False, **kwargs): + def __reduce_ex__(self, proto): + # We should not reattach the hooks when we are deep copying the test + new = functools.partial( + RegressionTest.__new__, _rfm_attach_hooks=False + ) + return new, (type(self),), self.__dict__ + + def __new__(cls, *args, + _rfm_use_params=False, _rfm_attach_hooks=True, **kwargs): obj = super().__new__(cls) # Insert the var & param spaces @@ -749,10 +758,16 @@ def __new__(cls, *args, _rfm_use_params=False, **kwargs): os.path.dirname(inspect.getfile(cls)) ) - # Apply the hooks only once (not when deep-copying) - if not hasattr(cls, '_rfm_first_init'): - cls._rfm_pipeline_hooks.apply(obj) - cls._rfm_first_init = True + if _rfm_attach_hooks: + cls._add_hooks('__init__') + cls._add_hooks('setup') + cls._add_hooks('compile', 'pre_compile') + cls._add_hooks('compile_wait', 'post_compile') + cls._add_hooks('run', 'pre_run') + cls._add_hooks('run_wait', 'post_run') + cls._add_hooks('sanity') + cls._add_hooks('performance') + cls._add_hooks('cleanup') # Initialize the test obj.__rfm_init__(name, prefix) @@ -768,6 +783,32 @@ def _append_parameters_to_name(self): else: return '' + @classmethod + def _add_hooks(cls, fn_name, kind=None): + if fn_name in cls.__dict__: + # If a parent test has been instantiated, this function has been + # replaced, so we need to remove any hooks from it, so that the + # hooks are not applied twice for the derived class + cls._remove_hooks(fn_name) + + pipeline_hooks = cls._rfm_pipeline_hooks + fn = getattr(cls, fn_name) + with contextlib.suppress(AttributeError): + fn = fn.__rfm_pipeline_fn__ + + new_fn = hooks.attach_hooks(pipeline_hooks, kind)(fn) + setattr(cls, fn_name, new_fn) + + @classmethod + def _remove_hooks(cls, fn_name): + for c in cls.mro(): + if c == cls or fn_name not in c.__dict__: + continue + + fn = getattr(c, fn_name) + if hasattr(fn, '__rfm_pipeline_fn__'): + setattr(c, fn_name, fn.__rfm_pipeline_fn__) + @classmethod def __init_subclass__(cls, *, special=False, pin_prefix=False, **kwargs): super().__init_subclass__(**kwargs) diff --git a/reframe/utility/__init__.py b/reframe/utility/__init__.py index 46a96c77b6..0d6f7d615e 100644 --- a/reframe/utility/__init__.py +++ b/reframe/utility/__init__.py @@ -883,7 +883,7 @@ def __repr__(self): if not vals: return type(self).__name__ + '()' else: - return '{' + ', '.join(repr(v) for v in vals) + '}' + return '{' + ', '.join(builtins.repr(v) for v in vals) + '}' # Container i/face def __contains__(self, item): diff --git a/unittests/test_ci.py b/unittests/test_ci.py index a3f221d24f..29810c4138 100644 --- a/unittests/test_ci.py +++ b/unittests/test_ci.py @@ -6,6 +6,7 @@ import io import jsonschema +import pytest import requests import yaml @@ -29,8 +30,12 @@ def test_ci_gitlab_pipeline(): pipeline = fp.getvalue() # Fetch the latest Gitlab CI JSON schema - response = requests.get('https://json.schemastore.org/gitlab-ci') - assert response.ok + try: + response = requests.get('https://json.schemastore.org/gitlab-ci') + except requests.exceptions.ConnectionError as e: + pytest.skip(f'could not reach URL: {e}') + else: + assert response.ok schema = response.json() jsonschema.validate(yaml.safe_load(pipeline), schema) diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 3a3f012853..225cb2efcc 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -689,6 +689,35 @@ class MyTest(DerivedTest): } +def test_inherited_hooks_from_instantiated_tests(HelloTest, local_exec_ctx): + @fixtures.custom_prefix('unittests/resources/checks') + class T0(HelloTest): + def __init__(self): + super().__init__() + self.name = type(self).__name__ + self.executable = os.path.join('.', self.name) + self.var = 0 + + @rfm.run_after('setup') + def x(self): + self.var += 1 + + class T1(T0): + @rfm.run_before('run') + def y(self): + self.foo = 1 + + t0 = T0() + t1 = T1() + print('==> running t0') + _run(t0, *local_exec_ctx) + print('==> running t1') + _run(t1, *local_exec_ctx) + assert t0.var == 1 + assert t1.var == 1 + assert t1.foo == 1 + + def test_overriden_hooks(HelloTest, local_exec_ctx): @fixtures.custom_prefix('unittests/resources/checks') class BaseTest(HelloTest): From d06bf2de78340a8daa41e3f990ce04c59d4a4a82 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 2 Apr 2021 00:08:56 +0200 Subject: [PATCH 08/16] Fix unit tests after merge --- reframe/core/pipeline.py | 1 + 1 file changed, 1 insertion(+) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index bc5d34108e..f9aa464004 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -13,6 +13,7 @@ ] +import contextlib import functools import glob import inspect From 89a4ebcc751887c635b54c3793060aa3504c080d Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 2 Apr 2021 00:20:31 +0200 Subject: [PATCH 09/16] Remove unused imports --- reframe/core/meta.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 9564306a0f..13f14dd889 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -8,14 +8,12 @@ # -import contextlib - import reframe.core.namespaces as namespaces import reframe.core.parameters as parameters import reframe.core.variables as variables from reframe.core.exceptions import ReframeSyntaxError -from reframe.core.hooks import Hook, HookRegistry +from reframe.core.hooks import HookRegistry class RegressionTestMeta(type): From 2d19132c5734cda065d1db706581b8889e4d87eb Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Fri, 2 Apr 2021 18:20:19 +0200 Subject: [PATCH 10/16] Overload __getattribute__ for the pipeline stages --- reframe/core/decorators.py | 1 + reframe/core/hooks.py | 5 --- reframe/core/pipeline.py | 64 ++++++++++++++++---------------------- 3 files changed, 27 insertions(+), 43 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index ebe807f40d..8a0fbaeb87 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -262,6 +262,7 @@ def __init__(self): Add the ability to define post-init hooks in tests. ''' + stage = stage if stage != 'init' else '__init__' return _runx('post_' + stage) diff --git a/reframe/core/hooks.py b/reframe/core/hooks.py index 77977d76b3..957cff50d0 100644 --- a/reframe/core/hooks.py +++ b/reframe/core/hooks.py @@ -24,9 +24,6 @@ def select_hooks(obj, kind): phase = name if phase is None: fn_name = func.__name__ - if fn_name == '__init__': - fn_name = 'init' - phase = kind + fn_name elif phase is not None and not phase.startswith(kind): # Just any phase that does not exist @@ -38,8 +35,6 @@ def select_hooks(obj, kind): return [h for h in hooks[phase] if h.__name__ not in obj._disabled_hooks] - func.__rfm_pipeline_fn__ = func - @functools.wraps(func) def _fn(obj, *args, **kwargs): for h in select_hooks(obj, 'pre_'): diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index f9aa464004..8ea18c9794 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -79,6 +79,22 @@ DEPEND_FULLY = 3 +#: Pipeline stages +#: +#: :meta private: +_rfm_pipeline_stages = { + '__init__': None, + 'setup': None, + 'compile': 'pre_compile', + 'compile_wait': 'post_compile', + 'run': 'pre_run', + 'run_wait': 'post_run', + 'sanity': None, + 'performance': None, + 'cleanup': None +} + + def final(fn): fn._rfm_final = True @@ -724,15 +740,7 @@ def pipeline_hooks(cls): #: :type: boolean : :default: :class:`True` build_locally = variable(bool, value=True) - def __reduce_ex__(self, proto): - # We should not reattach the hooks when we are deep copying the test - new = functools.partial( - RegressionTest.__new__, _rfm_attach_hooks=False - ) - return new, (type(self),), self.__dict__ - - def __new__(cls, *args, - _rfm_use_params=False, _rfm_attach_hooks=True, **kwargs): + def __new__(cls, *args, _rfm_use_params=False, **kwargs): obj = super().__new__(cls) # Insert the var & param spaces @@ -764,16 +772,9 @@ def __new__(cls, *args, os.path.dirname(inspect.getfile(cls)) ) - if _rfm_attach_hooks: - cls._add_hooks('__init__') - cls._add_hooks('setup') - cls._add_hooks('compile', 'pre_compile') - cls._add_hooks('compile_wait', 'post_compile') - cls._add_hooks('run', 'pre_run') - cls._add_hooks('run_wait', 'post_run') - cls._add_hooks('sanity') - cls._add_hooks('performance') - cls._add_hooks('cleanup') + # Attach the hooks to the pipeline stages + for key, value in _rfm_pipeline_stages.items(): + cls._add_hooks(key, value) # Initialize the test obj.__rfm_init__(name, prefix) @@ -791,29 +792,16 @@ def _append_parameters_to_name(self): @classmethod def _add_hooks(cls, fn_name, kind=None): - if fn_name in cls.__dict__: - # If a parent test has been instantiated, this function has been - # replaced, so we need to remove any hooks from it, so that the - # hooks are not applied twice for the derived class - cls._remove_hooks(fn_name) - pipeline_hooks = cls._rfm_pipeline_hooks fn = getattr(cls, fn_name) - with contextlib.suppress(AttributeError): - fn = fn.__rfm_pipeline_fn__ - new_fn = hooks.attach_hooks(pipeline_hooks, kind)(fn) - setattr(cls, fn_name, new_fn) + setattr(cls, '_rfm_decorated_'+fn_name, new_fn) - @classmethod - def _remove_hooks(cls, fn_name): - for c in cls.mro(): - if c == cls or fn_name not in c.__dict__: - continue - - fn = getattr(c, fn_name) - if hasattr(fn, '__rfm_pipeline_fn__'): - setattr(c, fn_name, fn.__rfm_pipeline_fn__) + def __getattribute__(self, name): + if name in _rfm_pipeline_stages: + name = f'_rfm_decorated_{name}' + + return super().__getattribute__(name) @classmethod def __init_subclass__(cls, *, special=False, pin_prefix=False, **kwargs): From c090bc7c67407315e9277cb08e484ae7f6c55d6b Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Fri, 2 Apr 2021 18:38:57 +0200 Subject: [PATCH 11/16] Remove unused imports --- reframe/core/pipeline.py | 1 - 1 file changed, 1 deletion(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 8ea18c9794..2fa88d3f9e 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -13,7 +13,6 @@ ] -import contextlib import functools import glob import inspect From b485d9df7c5b92f7822972fa2c92b5f9ca7b5ee6 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sat, 3 Apr 2021 19:58:43 +0200 Subject: [PATCH 12/16] Further implementation improvements --- reframe/core/decorators.py | 10 +++++++++- reframe/core/hooks.py | 11 ++--------- reframe/core/pipeline.py | 40 +++++++++++++++++--------------------- 3 files changed, 29 insertions(+), 32 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 8a0fbaeb87..d95750b836 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -262,7 +262,15 @@ def __init__(self): Add the ability to define post-init hooks in tests. ''' - stage = stage if stage != 'init' else '__init__' + + # Map user stage names to the actual pipeline functions if needed + if stage == 'init': + stage = '__init__' + elif stage == 'compile': + stage = 'compile_wait' + elif stage == 'run': + stage = 'run_wait' + return _runx('post_' + stage) diff --git a/reframe/core/hooks.py b/reframe/core/hooks.py index 957cff50d0..ccca205486 100644 --- a/reframe/core/hooks.py +++ b/reframe/core/hooks.py @@ -9,7 +9,7 @@ import reframe.utility as util -def attach_hooks(hooks, name=None): +def attach_hooks(hooks): '''Attach pipeline hooks to phase ``name''. This function returns a decorator for pipeline functions that will run the @@ -21,14 +21,7 @@ def attach_hooks(hooks, name=None): def _deco(func): def select_hooks(obj, kind): - phase = name - if phase is None: - fn_name = func.__name__ - phase = kind + fn_name - elif phase is not None and not phase.startswith(kind): - # Just any phase that does not exist - phase = 'xxx' - + phase = kind + func.__name__ if phase not in hooks: return [] diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 2fa88d3f9e..74af38471d 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -78,20 +78,15 @@ DEPEND_FULLY = 3 -#: Pipeline stages -#: -#: :meta private: -_rfm_pipeline_stages = { - '__init__': None, - 'setup': None, - 'compile': 'pre_compile', - 'compile_wait': 'post_compile', - 'run': 'pre_run', - 'run_wait': 'post_run', - 'sanity': None, - 'performance': None, - 'cleanup': None -} +_PIPELINE_STAGES = ( + '__init__', + 'setup', + 'compile', 'compile_wait', + 'run', 'run_wait', + 'sanity', + 'performance', + 'cleanup' +) def final(fn): @@ -772,8 +767,9 @@ def __new__(cls, *args, _rfm_use_params=False, **kwargs): ) # Attach the hooks to the pipeline stages - for key, value in _rfm_pipeline_stages.items(): - cls._add_hooks(key, value) + print(cls._rfm_pipeline_hooks) + for stage in _PIPELINE_STAGES: + cls._add_hooks(stage) # Initialize the test obj.__rfm_init__(name, prefix) @@ -790,15 +786,15 @@ def _append_parameters_to_name(self): return '' @classmethod - def _add_hooks(cls, fn_name, kind=None): + def _add_hooks(cls, stage): pipeline_hooks = cls._rfm_pipeline_hooks - fn = getattr(cls, fn_name) - new_fn = hooks.attach_hooks(pipeline_hooks, kind)(fn) - setattr(cls, '_rfm_decorated_'+fn_name, new_fn) + fn = getattr(cls, stage) + new_fn = hooks.attach_hooks(pipeline_hooks)(fn) + setattr(cls, '_rfm_pipeline_fn_' + stage, new_fn) def __getattribute__(self, name): - if name in _rfm_pipeline_stages: - name = f'_rfm_decorated_{name}' + if name in _PIPELINE_STAGES: + name = f'_rfm_pipeline_fn_{name}' return super().__getattribute__(name) From 9ac7e9f4346fa75ac11aeb42fc1660e0d6985124 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sat, 3 Apr 2021 20:05:43 +0200 Subject: [PATCH 13/16] Properly restore runtime in `test_policies.py` unit tests --- reframe/core/pipeline.py | 1 - unittests/test_cli.py | 5 +++-- unittests/test_dependencies.py | 5 +++-- unittests/test_logging.py | 2 +- unittests/test_pipeline.py | 5 +++-- unittests/test_policies.py | 18 ++++++++++++++---- unittests/test_schedulers.py | 5 +++-- unittests/test_utility.py | 7 ++++--- 8 files changed, 31 insertions(+), 17 deletions(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 6db994b39c..f01bfe628d 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -767,7 +767,6 @@ def __new__(cls, *args, _rfm_use_params=False, **kwargs): ) # Attach the hooks to the pipeline stages - print(cls._rfm_pipeline_hooks) for stage in _PIPELINE_STAGES: cls._add_hooks(stage) diff --git a/unittests/test_cli.py b/unittests/test_cli.py index fff6223531..5c54b005e0 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -108,10 +108,11 @@ def _run_reframe(system='generic:default', @pytest.fixture def temp_runtime(tmp_path): - def _temp_runtime(site_config, system=None, options={}): + def _temp_runtime(site_config, system=None, options=None): + options = options or {} options.update({'systems/prefix': tmp_path}) with rt.temp_runtime(site_config, system, options): - yield rt.runtime + yield yield _temp_runtime diff --git a/unittests/test_dependencies.py b/unittests/test_dependencies.py index e1db9c54e4..53784f253f 100644 --- a/unittests/test_dependencies.py +++ b/unittests/test_dependencies.py @@ -85,10 +85,11 @@ def find_case(cname, ename, partname, cases): @pytest.fixture def temp_runtime(tmp_path): - def _temp_runtime(site_config, system=None, options={}): + def _temp_runtime(site_config, system=None, options=None): + options = options or {} options.update({'systems/prefix': tmp_path}) with rt.temp_runtime(site_config, system, options): - yield rt.runtime + yield yield _temp_runtime diff --git a/unittests/test_logging.py b/unittests/test_logging.py index e47e06e799..bb59f65f5d 100644 --- a/unittests/test_logging.py +++ b/unittests/test_logging.py @@ -226,7 +226,7 @@ def _temp_runtime(logging_config): fp.write(f'site_configuration = {util.ppretty(site_config)}') with rt.temp_runtime(fp.name): - yield rt.runtime() + yield return _temp_runtime diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 1937a35178..f4152e602d 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -56,10 +56,11 @@ def pinnedtest(): @pytest.fixture def temp_runtime(tmp_path): - def _temp_runtime(config_file, system=None, options={}): + def _temp_runtime(config_file, system=None, options=None): + options = options or {} options.update({'systems/prefix': str(tmp_path)}) with rt.temp_runtime(config_file, system, options): - yield rt.runtime() + yield yield _temp_runtime diff --git a/unittests/test_policies.py b/unittests/test_policies.py index f6b3fe3862..ba9dd7ad34 100644 --- a/unittests/test_policies.py +++ b/unittests/test_policies.py @@ -63,10 +63,11 @@ def timestamps(self): @pytest.fixture def temp_runtime(tmp_path): - def _temp_runtime(site_config, system=None, options={}): + def _temp_runtime(site_config, system=None, options=None): + options = options or {} options.update({'systems/prefix': str(tmp_path)}) with rt.temp_runtime(site_config, system, options): - yield rt.runtime + yield yield _temp_runtime @@ -819,6 +820,9 @@ def test_compile_fail_reschedule_main_loop(async_runner, make_cases, assert_runall(runner) assert num_checks == len(stats.failed()) + with contextlib.suppress(StopIteration): + next(ctx) + def test_compile_fail_reschedule_busy_loop(async_runner, make_cases, make_async_exec_ctx): @@ -834,6 +838,8 @@ def test_compile_fail_reschedule_busy_loop(async_runner, make_cases, assert num_checks == stats.num_cases() assert_runall(runner) assert num_checks == len(stats.failed()) + with contextlib.suppress(StopIteration): + next(ctx) @pytest.fixture @@ -875,8 +881,12 @@ def test_restore_session(report_file, make_runner, assert new_report['runs'][0]['testcases'][0]['name'] == 'T1' # Remove the test case dump file and retry - os.remove(tmp_path / 'stage' / 'generic' / 'default' / - 'builtin' / 'T4' / '.rfm_testcase.json') + try: + os.remove(tmp_path / 'stage' / 'generic' / 'default' / + 'builtin' / 'T4' / '.rfm_testcase.json') + except OSError: + import reframe.utility as util + print('==> dep_cases', util.repr(dep_cases)) with pytest.raises(ReframeError, match=r'could not restore testcase'): report.restore_dangling(testgraph) diff --git a/unittests/test_schedulers.py b/unittests/test_schedulers.py index e01003a15c..3084d3bda1 100644 --- a/unittests/test_schedulers.py +++ b/unittests/test_schedulers.py @@ -45,10 +45,11 @@ def local_only(scheduler): @pytest.fixture def temp_runtime(tmp_path): - def _temp_runtime(site_config, system=None, options={}): + def _temp_runtime(site_config, system=None, options=None): + options = options or {} options.update({'systems/prefix': tmp_path}) with rt.temp_runtime(site_config, system, options): - yield rt.runtime + yield yield _temp_runtime diff --git a/unittests/test_utility.py b/unittests/test_utility.py index 2350ad1762..f322707d15 100644 --- a/unittests/test_utility.py +++ b/unittests/test_utility.py @@ -1442,10 +1442,11 @@ def test_cray_cle_info_missing_parts(tmp_path): @pytest.fixture def temp_runtime(tmp_path): - def _temp_runtime(site_config, system=None, options={}): + def _temp_runtime(site_config, system=None, options=None): + options = options or {} options.update({'systems/prefix': tmp_path}) - with rt.temp_runtime(site_config, system, options) as ctx: - yield ctx + with rt.temp_runtime(site_config, system, options): + yield yield _temp_runtime From 59e9f573bc46668660ab25794c671d2db61556fb Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 6 Apr 2021 19:00:29 +0200 Subject: [PATCH 14/16] Better implementation of the `make_async_exec_ctx` fixture And apply the fix in all tests in `test_policies.py`. --- unittests/test_policies.py | 59 ++++++++++++++------------------------ 1 file changed, 21 insertions(+), 38 deletions(-) diff --git a/unittests/test_policies.py b/unittests/test_policies.py index ba9dd7ad34..56282d4a23 100644 --- a/unittests/test_policies.py +++ b/unittests/test_policies.py @@ -554,11 +554,18 @@ def on_task_setup(self, task): @pytest.fixture def make_async_exec_ctx(temp_runtime): + tmprt = None + def _make_async_exec_ctx(max_jobs): - yield from temp_runtime(fixtures.TEST_CONFIG_FILE, 'generic', - {'systems/partitions/max_jobs': max_jobs}) + nonlocal tmprt + tmprt = temp_runtime(fixtures.TEST_CONFIG_FILE, 'generic', + {'systems/partitions/max_jobs': max_jobs}) + next(tmprt) + + yield _make_async_exec_ctx - return _make_async_exec_ctx + with contextlib.suppress(StopIteration): + next(tmprt) @pytest.fixture @@ -592,8 +599,7 @@ def test_concurrency_unlimited(async_runner, make_cases, make_async_exec_ctx): num_checks = 3 # Trigger evaluation of the execution context - ctx = make_async_exec_ctx(num_checks) - next(ctx) + make_async_exec_ctx(max_jobs=num_checks) runner, monitor = async_runner runner.runall(make_cases([SleepCheck(.5) for i in range(num_checks)])) @@ -620,8 +626,7 @@ def test_concurrency_unlimited(async_runner, make_cases, make_async_exec_ctx): def test_concurrency_limited(async_runner, make_cases, make_async_exec_ctx): # The number of checks must be <= 2*max_jobs. num_checks, max_jobs = 5, 3 - ctx = make_async_exec_ctx(max_jobs) - next(ctx) + make_async_exec_ctx(max_jobs) runner, monitor = async_runner runner.runall(make_cases([SleepCheck(.5) for i in range(num_checks)])) @@ -662,8 +667,7 @@ def test_concurrency_limited(async_runner, make_cases, make_async_exec_ctx): def test_concurrency_none(async_runner, make_cases, make_async_exec_ctx): num_checks = 3 - ctx = make_async_exec_ctx(1) - next(ctx) + make_async_exec_ctx(max_jobs=1) runner, monitor = async_runner runner.runall(make_cases([SleepCheck(.5) for i in range(num_checks)])) @@ -703,9 +707,7 @@ def assert_interrupted_run(runner): def test_kbd_interrupt_in_wait_with_concurrency(async_runner, make_cases, make_async_exec_ctx): - ctx = make_async_exec_ctx(4) - next(ctx) - + make_async_exec_ctx(max_jobs=4) runner, _ = async_runner with pytest.raises(KeyboardInterrupt): runner.runall(make_cases([ @@ -723,9 +725,7 @@ def test_kbd_interrupt_in_wait_with_limited_concurrency( # KeyboardInterruptCheck to finish first (the corresponding wait should # trigger the failure), so as to make the framework kill the remaining # three. - ctx = make_async_exec_ctx(2) - next(ctx) - + make_async_exec_ctx(max_jobs=2) runner, _ = async_runner with pytest.raises(KeyboardInterrupt): runner.runall(make_cases([ @@ -738,9 +738,7 @@ def test_kbd_interrupt_in_wait_with_limited_concurrency( def test_kbd_interrupt_in_setup_with_concurrency(async_runner, make_cases, make_async_exec_ctx): - ctx = make_async_exec_ctx(4) - next(ctx) - + make_async_exec_ctx(max_jobs=4) runner, _ = async_runner with pytest.raises(KeyboardInterrupt): runner.runall(make_cases([ @@ -753,9 +751,7 @@ def test_kbd_interrupt_in_setup_with_concurrency(async_runner, make_cases, def test_kbd_interrupt_in_setup_with_limited_concurrency( async_runner, make_cases, make_async_exec_ctx): - ctx = make_async_exec_ctx(2) - next(ctx) - + make_async_exec_ctx(max_jobs=2) runner, _ = async_runner with pytest.raises(KeyboardInterrupt): runner.runall(make_cases([ @@ -768,9 +764,7 @@ def test_kbd_interrupt_in_setup_with_limited_concurrency( def test_run_complete_fails_main_loop(async_runner, make_cases, make_async_exec_ctx): - ctx = make_async_exec_ctx(1) - next(ctx) - + make_async_exec_ctx(max_jobs=1) runner, _ = async_runner num_checks = 3 runner.runall(make_cases([SleepCheckPollFail(10), @@ -788,9 +782,7 @@ def test_run_complete_fails_main_loop(async_runner, make_cases, def test_run_complete_fails_busy_loop(async_runner, make_cases, make_async_exec_ctx): - ctx = make_async_exec_ctx(1) - next(ctx) - + make_async_exec_ctx(max_jobs=1) runner, _ = async_runner num_checks = 3 runner.runall(make_cases([SleepCheckPollFailLate(1), @@ -808,9 +800,7 @@ def test_run_complete_fails_busy_loop(async_runner, make_cases, def test_compile_fail_reschedule_main_loop(async_runner, make_cases, make_async_exec_ctx): - ctx = make_async_exec_ctx(1) - next(ctx) - + make_async_exec_ctx(max_jobs=1) runner, _ = async_runner num_checks = 2 runner.runall(make_cases([SleepCheckPollFail(.1), CompileFailureCheck()])) @@ -820,15 +810,10 @@ def test_compile_fail_reschedule_main_loop(async_runner, make_cases, assert_runall(runner) assert num_checks == len(stats.failed()) - with contextlib.suppress(StopIteration): - next(ctx) - def test_compile_fail_reschedule_busy_loop(async_runner, make_cases, make_async_exec_ctx): - ctx = make_async_exec_ctx(1) - next(ctx) - + make_async_exec_ctx(max_jobs=1) runner, _ = async_runner num_checks = 2 runner.runall( @@ -838,8 +823,6 @@ def test_compile_fail_reschedule_busy_loop(async_runner, make_cases, assert num_checks == stats.num_cases() assert_runall(runner) assert num_checks == len(stats.failed()) - with contextlib.suppress(StopIteration): - next(ctx) @pytest.fixture From 85bbc9ca7e88610a66503647769a7e9ba47c397e Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 6 Apr 2021 20:13:51 +0200 Subject: [PATCH 15/16] Address PR comments (II) --- unittests/test_pipeline.py | 20 ++++++++++---------- unittests/test_policies.py | 8 ++------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index f4152e602d..40b7b23865 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -537,29 +537,29 @@ def prepare(self): def test_post_init_hook(local_exec_ctx): class _T0(rfm.RunOnlyRegressionTest): - x = variable(int, value=0) - y = variable(int, value=1) + x = variable(str, value='y') + y = variable(str, value='x') def __init__(self): - self.x = 1 + self.x = 'x' @rfm.run_after('init') def prepare(self): - self.y += 1 + self.y += 'y' class _T1(_T0): def __init__(self): super().__init__() - self.z = 3 + self.z = 'z' t0 = _T0() - assert t0.x == 1 - assert t0.y == 2 + assert t0.x == 'x' + assert t0.y == 'xy' t1 = _T1() - assert t1.x == 1 - assert t1.y == 2 - assert t1.z == 3 + assert t1.x == 'x' + assert t1.y == 'xy' + assert t1.z == 'z' def test_setup_hooks(HelloTest, local_exec_ctx): diff --git a/unittests/test_policies.py b/unittests/test_policies.py index 56282d4a23..6d06f738ce 100644 --- a/unittests/test_policies.py +++ b/unittests/test_policies.py @@ -864,12 +864,8 @@ def test_restore_session(report_file, make_runner, assert new_report['runs'][0]['testcases'][0]['name'] == 'T1' # Remove the test case dump file and retry - try: - os.remove(tmp_path / 'stage' / 'generic' / 'default' / - 'builtin' / 'T4' / '.rfm_testcase.json') - except OSError: - import reframe.utility as util - print('==> dep_cases', util.repr(dep_cases)) + os.remove(tmp_path / 'stage' / 'generic' / 'default' / + 'builtin' / 'T4' / '.rfm_testcase.json') with pytest.raises(ReframeError, match=r'could not restore testcase'): report.restore_dangling(testgraph) From ebd2ea0097e63c45c0f98921c7fbc579576d14c2 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 6 Apr 2021 20:58:27 +0200 Subject: [PATCH 16/16] Raise `ValueError` for invalid pipeline stages --- reframe/core/decorators.py | 13 +++++++++++++ unittests/test_pipeline.py | 19 +++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index df66a6c4ab..f7a4085b9d 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -214,6 +214,13 @@ def _fn(*args, **kwargs): return deco +# Valid pipeline stages that users can specify in the `run_before()` and +# `run_after()` decorators +_USER_PIPELINE_STAGES = ( + 'init', 'setup', 'compile', 'run', 'sanity', 'performance', 'cleanup' +) + + def run_before(stage): '''Decorator for attaching a test method to a pipeline stage. @@ -227,6 +234,9 @@ def run_before(stage): ``'run'``, ``'sanity'``, ``'performance'`` or ``'cleanup'``. ''' + if stage not in _USER_PIPELINE_STAGES: + raise ValueError(f'invalid pipeline stage specified: {stage!r}') + if stage == 'init': raise ValueError('pre-init hooks are not allowed') @@ -263,6 +273,9 @@ def __init__(self): ''' + if stage not in _USER_PIPELINE_STAGES: + raise ValueError(f'invalid pipeline stage specified: {stage!r}') + # Map user stage names to the actual pipeline functions if needed if stage == 'init': stage = '__init__' diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 40b7b23865..d74e80dadf 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -526,9 +526,24 @@ def set_resources(self): assert expected_job_options == set(test.job.options) -def test_pre_init_hook(local_exec_ctx): +def test_unkown_pre_hook(): + with pytest.raises(ValueError): + class MyTest(rfm.RunOnlyRegressionTest): + @rfm.run_before('foo') + def prepare(self): + self.x = 1 + + +def test_unkown_post_hook(): + with pytest.raises(ValueError): + class MyTest(rfm.RunOnlyRegressionTest): + @rfm.run_after('foo') + def prepare(self): + self.x = 1 + + +def test_pre_init_hook(): with pytest.raises(ValueError): - @fixtures.custom_prefix('unittests/resources/checks') class MyTest(rfm.RunOnlyRegressionTest): @rfm.run_before('init') def prepare(self):