[feat] Re-implement pipeline hook mechanism and add a post-init hook

[vkarak]

With the new syntax of variable definition we can almost get rid of __init__(). Currently, functions like depends_on() must still be called during initialisation, but this will be addressed by #1864. However, there will still be cases that something must be executed during the initialisation and this is when you want to have dynamic tags, for example, change the tags based on a test parameter. In order for the frontend to be able to select tests by tags, the tags must be finalised after the initialisation, so you can't practically set them in any other pipeline stage.

The post-init hook introduced here is fairly simple it can substitute the constructor. The following:

class my_test(...):
    @rfm.run_after('init')
    def init_a(self):
        self.a = 1

    @rfm.run_after('init')
    def init_a(self):
        self.b = 2

is equivalent to

class my_test(...):
    def __init__(self):
        self.a = 1
        self.b = 2

Of course, the example with init_a() and init_b() is a bit silly, but it gets more sense if you imagine test hierarchies.
If a test defines __init__(), any init hooks will be ignored.

Finally, there are no pre-init hooks, although it's trivial to implement. I'm not introducing them here, unless there is a need.

Fixes #1863.

Implementation details

The pipeline mechanism is re-implemented from scratch. Pipeline hooks are no more applied to the pipeline functions with the decorator. This has a number of 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.

Specific implementation tricks and intricacies are explained with comments in the code.

[victorusu]

lgtm. I just wonder if it makes sense to tell the difference between the run_before('setup'), compared to run_after('init'). Or in other words, what is the init hook meant to be used for.

[jjotero]

If a test defines init(), any init hooks will be ignored.

I find very very confusing that a standard Python method can magically override a reframe hook. I'm struggling to see what the advantage is of having this post-init hook. Is this simply to get rid of __init__?

[vkarak]

I'm struggling to see what the advantage is of having this post-init hook. Is this simply to get rid of init?

Basically, yes. It is just syntactic sugar. Generally, everything can go into hooks, but there is one case that you can't use hooks: it's when you have to deal with test filtering. There are tests that we change the tags or the valid_prog_environs based on some parameter. This cannot happen anywhere else except in init, so there will be tests to define it, if you don't have some sort of hooks. What about defining the hook differently, something like pre_filter. I don't know.

[vkarak]

I will postpone this PR to the next sprint, since I would like to improve also the current hook mechanism to follow the same pattern as with what we have discussed with __init__(), so that the hooks run even if a method has been overriden and make sure that they are executed once at the class lowest in the hierarchy that defines the hooked method.

[jjotero]

I like the idea of the HookRegistry, but I'm not convinced by the new way the pipeline stages are dynamically decorated to pick up the hooks. I see no reason to do that and I feel that it makes more sense to have the pipeline stages decorated in the pipeline itself. See comments below.

[vkarak]

I see no reason to do that and I feel that it makes more sense to have the pipeline stages decorated in the pipeline itself. See comments below.

The "problem" is that we need to handle disabled hooks. Users can disable hooks on-demand and this happens after the test has been created. Regardless if the _rfm_disabled_hooks is a class or instance attribute, it is set only after the object is created, i.e., after __rfm_init__(). For this reason, running the hooks cannot be static and I also converted the _rfm_disabled_hooks to an object attribute, because it didn't have any meaning to be a class attribute. It was only complicating the implementation of inheritance.

[pep8speaks]

Hello @vkarak, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2021-04-06 18:59:34 UTC

[codecov-io]

Codecov Report

Merging #1865 (985d774) into master (725c78f) will increase coverage by 0.00%.
The diff coverage is 95.61%.

@@           Coverage Diff           @@
##           master    #1865   +/-   ##
=======================================
  Coverage   87.90%   87.91%           
=======================================
  Files          49       50    +1     
  Lines        8451     8491   +40     
=======================================
+ Hits         7429     7465   +36     
- Misses       1022     1026    +4     

Impacted Files	Coverage Δ
reframe/frontend/cli.py	75.98% <0.00%> (ø)
reframe/core/hooks.py	94.59% <94.59%> (ø)
reframe/core/decorators.py	97.27% <100.00%> (+0.21%)	⬆️
reframe/core/meta.py	100.00% <100.00%> (ø)
reframe/core/pipeline.py	91.57% <100.00%> (-0.43%)	⬇️
reframe/utility/__init__.py	92.12% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 725c78f...985d774. Read the comment docs.

[jjotero]

lgtm

[vkarak]

I fixed the unit tests in test_policies.py, but this problem is in different files, too, but it doesn't get triggered. I have created #1913 to address this separately.

[vkarak]

@jenkins-cscs retry all