[feat] Make hook-related machinery available as class directives

[jjotero]

This PR builds on #1929 to simplify and improve the syntax when writing test libraries. The run_before, run_after and require_deps decorators are now available as class directives, so there is no need for the preceding rfm. anymore:

class Foo(rfm.RegressionTest):
    ...
    @run_before('compile')
    def a_pre_compile_hook(self):
        # do something

    # The standard way is still supported
    @rfm.run_after('run')
    def a_post_run_hook(self):
        # do something else

This PR also introduces the bind directive, which binds an external function with the regression test class. This allows to write pipeline hooks as standalone unary functions, which can then be safely "imported" by several other classes.

# file handy_hooks.py
def ext_fn(x):
    # do something handy

# file mytest.py
from handy_hooks import ext_fn

class MyTest(rfm.RegressionTest):
    run_before('compile')(bind(ext_fn))

    # It is possible to bind the hook with a different name
    run_after('run')(bind(ext_fn, name='new_name'))

Not using the bind directive here might lead to problems because the function object ext_fn would be shared by multiple classes.

Issues with the current implementation

By moving these directives into the reframe/core/hooks.py I feel that the mapping of the argument to the run_before and run_after should be done in the metaclass and not by any of the functions in the hooks.py file. It's a bit strange to me that these hook functions are aware of the pipeline stages. It would feel a lot more natural if the directives exposed by the metaclass would preprocess this argument and do the mapping before calling any of the functions in hooks.py.

Alternatively, we could add some syntactic sugar into this and get rid of the mapping problem altogether by exposing directives to the pre_ and post_ steps of each individual pipeline stage. IMO, this would vastly improve the syntax above:

from handy_hooks import ext_fn

class MyTest(rfm.RegressionTest):
    pre_compile(bind(ext_fn))
    post_run(bind(ext_fn, name='new_name'))

[pep8speaks]

Hello @jjotero, 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-06-07 08:19:44 UTC

[codecov-commenter]

Codecov Report

Merging #1969 (e0f2810) into master (41d5492) will decrease coverage by 0.15%.
The diff coverage is 89.24%.

@@            Coverage Diff             @@
##           master    #1969      +/-   ##
==========================================
- Coverage   87.61%   87.46%   -0.16%     
==========================================
  Files          50       50              
  Lines        8788     8850      +62     
==========================================
+ Hits         7700     7741      +41     
- Misses       1088     1109      +21     

Impacted Files	Coverage Δ
reframe/core/decorators.py	64.70% <25.00%> (-18.49%)	⬇️
reframe/core/hooks.py	92.78% <92.00%> (-1.82%)	⬇️
reframe/core/meta.py	98.80% <96.66%> (-1.20%)	⬇️

---

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 41d5492...e0f2810. Read the comment docs.

[victorusu]

@jjotero, I am not sure how this feature will evolve or be utilized...
I understand that there is potential to reuse hooks. I do not see any other benefit and I wonder if the gain compensates for the costs...
Do you have a proper use case for this feature?
Because when I check the cscs-checks folder, I see that the hooks are test-dependent and cannot be used in any other check. Ok, this may be because we miss this feature, but at first glance, I cannot find any test that would really benefit from "hook reuse". It is possible that in my search I may have missed the key tests that would benefit, but still, it looks to me that they are not the majority.

So, what's the other benefit of this feature?

Furthermore, I agree that the syntactic-sugar syntax is required. And I wonder if, in the simplified form, there could be an implicit bind call. So, instead of having

from handy_hooks import ext_fn

class MyTest(rfm.RegressionTest):
    pre_compile(bind(ext_fn))
    post_run(bind(ext_fn, name='new_name'))

one could write

from handy_hooks import ext_fn

class MyTest(rfm.RegressionTest):
    pre_compile(ext_fn)
    post_run(ext_fn, name='new_name')

[jjotero]

So, what's the other benefit of this feature?

In the gpu microbenchmarks, the dgemm, shmem, kernel_latency, pointer_chase, memory_bandwidth and gpu_burn checks share the two hooks in /cscs-checks/microbenchmarks/hooks.py. In the current version, these checks manually bind the external hook by wrapping the external function; but that's a lot of typing :) So there the bind directive becomes very handy.

There is quite a lot of room at the hpctestlib level to use the bind directive there also on the same microbenchmarks from above. All the GPU microbenchmarks there also share some hooks.

Regarding the syntax, I agree that binding an external hook doing

class MyTest(rfm.RegressionTest):
    run_before('compile')(bind(ext_fn, name='local_fn'))

is perhaps worse than a manual bind such as

class MyTest(rfm.RegressionTest):
    @run_before('compile')
    def local_fn(self):
        ext_fn(self)

So that's why I think that the least we should do here is expose the pre_ & post_ decorators bound to a specific stage so we can do

from handy_hooks import ext_fn

class MyTest(rfm.RegressionTest):
    pre_compile(bind(ext_fn))
    post_run(bind(ext_fn, name='new_name'))

If you want to go further and make the bind transparent to the user, I'm fine with that, but I'd probably go for a more explicit name such as this

from handy_hooks import ext_fn

class MyTest(rfm.RegressionTest):
    inject_before_compile(ext_fn)
    inject_after_run(ext_fn, name='new_name'))

or instead

from handy_hooks import ext_fn

class MyTest(rfm.RegressionTest):
    inject_before('compile', ext_fn)
    inject_after('run', ext_fn, name='new_name'))

[jjotero]

Here's another and perhaps a bit of a different use-case. Look at the affinity_check. It's got a few "utility" functions in there such as get_sibling_cpus.
https://github.com/eth-cscs/reframe/blob/8bd06ea60a55b00c31c1fc999749ad3619823148/cscs-checks/prgenv/affinity_check.py#L154
If this function is factored out into a test utils library, other tests could use it as follows

from hpctestlib.utils.topo import get_sibling_cpus

@rfm.simple_test
class MyTest(rfm.RegressionTest):
    ...
    bind(get_sibling_cpus)

    @run_before('run')
    def get_logical_cpus_per_core(self):
        self.num_cpus_per_core = len(self.get_sibling_cpus(0, by='core'))

[vkarak]

I personally see the value of this proposal in this PR. Here are my general comments:

1. Do we still allow the rfm.run_before syntax?
2. I would vote against yet another syntax with the same meaning, such as pre_compile, inject_pre_compile etc. It will create more confusion. I like that the current proposal uses the same syntax and functionality: run_before() does exactly what it is expected to do as a decorator.
3. I don't see a problem with the bind() function: it takes a free function accepting at least one argument and binds it to the object.
4. I wouldn't argue that the one liner syntax is worse than the multiline, but again this is a matter of taste and, in fact, users can choose which one to use! I would definitely be one of those choosing the one liner.
5. I think adding the option to set the name of the bounded function is good, because it would avoid conflicts with other local methods.

[jjotero]

1. I'd vote to schedule the drop for 4.0.
2. I think the current run_before & run_after decorators would make the one-liner syntax very (very) hard to read for non-Python people. When you use a decorator as @deco, all you need to know is that this decorator does something to the function below and the user might not know/care how that happens. However, if you use it as deco(...)(...), the user must know how decorators work in order to understand the syntax. That's something far more advanced, I think. In any case, I think that the syntax we use for the decorators is a full discussion on its own (I agree that the inject syntax probably causes more problems than solutions). So I'd say we get this PR done and we have that chat at some other time :)

[vkarak]

I think the current run_before & run_after decorators would make the one-liner syntax very (very) hard to read for non-Python people. When you use a decorator as @Deco, all you need to know is that this decorator does something to the function below and the user might not know/care how that happens.

That's why there is the more verbose option to define a function and decorate it. I don't see the point in exposing the hooks as pre_compile, post_compile etc... That would mean another major syntax change with very little value and without a strong reason.

[vkarak]

I'd vote to schedule the drop for 4.0.

Meanwhile, we need to deprecate it since 3.7.0, I guess.

[jjotero]

Meanwhile, we need to deprecate it since 3.7.0, I guess.

Done

I've also intercepted all these directives to not make their way into the class and the instance, since something like self.bind(...) or self.parameter(...) does not make sense.

[vkarak]

I think there is sth wrong with this PR git-wise. I see 67 files changed 🤔

[jjotero]

@vkarak I've updated all the @rfm.run_before and so on to the @run_before syntax in all the cscs-checks. I can revert that if you think that should go in a separate PR.

[vkarak]

Oh, I see! No, keep it, because we don't want tons of deprecation warnings. We already have enough with the parameterized_test decorator :-)

[vkarak]

Lgtm in general, but I don't see unit tests for the new functionality introduced:

• The bind() function
• Removal of the blacklisted directives after the class is created.

[vkarak]

Still a couple of minor comments.

[jjotero]

Implementation-wise, I'm still not 100% happy that the file hooks.py is aware of the pipeline stages. Perhaps it would make sense to have the run_after and run_before functions specialising the _runx function inside the metaclass.

[jjotero]

I feel this now looks a lot better. The backend _runx function to attach the hooks to the stages has now been renamed to attach_to. Decorators run_before and run_after now live in the metaclass, and they use the attach_to function after mapping the pipeline stages.

[vkarak]

Lgtm, except the documentation needs a little bit of fine-tuning. Also the fact that the run_before etc. docs are in three places. I am working on that.

[vkarak]

@jjotero I've fine tuned the documentation as follows:

• Remove duplicate documentation; use pointers in the code to the actual user documentation.
• Move pipeline hooks section under the builtins.
• Add a warning box for the deprecation of former syntax.
• Other style fixes.

If you agree with the changes, I am ready to merge it.

[jjotero]

If you agree with the changes, I am ready to merge it.

Yeah, docs look far cleaner now.

I've also made the parameters and variables to show as RegressionMixin.parameter (instead of RegressionTest.parameter) for consistency with all the other builtins.