[feat] Introduce new syntax for allowing the creation of extensible parametrised tests

[jjotero]

Implementation of a new way to write more flexible parametrised tests (see #1567). This new approach does not rely on the test parameters to be passed as arguments to the __init__ method. Instead, the parameters are defined through class attribute directives that append these parameters to the namespace of the class. This allows extending the parameter space directly through class inheritance, which was not possible before. This method to write parametrised test introduces the concept of abstract parametrised tests. This would be tests with at least one parameter that does not have any values assigned to it. See this examples below.

class A(rfm.RegressionTest):                                                                                                          
    '''                                                                                                                                       
    This is a regular non-parametrised test                                                                                                           
    '''                                                                                                                                       
    def __init__(self):                                                                                                                       
        self.valid_systems = ['*']                                                                                                            
        self.valid_prog_environs = ['*']                                                                                                      
        self.sourcepath = 'hello.cpp'                                                                                                         
        self.sanity_patterns = sn.assert_found(r'Hello, World\!', self.stdout)                                                                
                                                                                                                                              
                                                                                                                                              
@rfm.test                                                                                                                                     
class B(A):                                                                                                                
    '''                                                                                                                                       
    This is now a parametrised test. The @rfm.test decorator registers the test.                                                              
    This will result into an all to all combination of the parameter space.                                                                   
    '''                                                                                                                                       
    rfm_parameter('P0', ['a', 'b', 'c'])                                                                                                      
    rfm_parameter('P1', ['1', '2'])                                                                                                           
    def __init__(self):                                                                                                                       
        super().__init__()                                                                                                                    
        self.build_system = 'SingleSource'                                                                                                    
        self.build_system.cxxflags = [r'-DFLAG1="\"%s\""' % self.P0,                                                                          
                                      r'-DFLAG2="\"%s\""' % self.P1,                                                                          
                                     ]                                                                                                        
                                                                                                                                              
                                                                                                                                              
class C(B):                                                                                                             
    '''                                                                                                                                       
    Make the test an abstract test - this will raise an error if decorated with @rfm.test                                                     
    '''                                                                                                                                       
    # P0 is an abstract parameter.                                                                                                            
    rfm_parameter('P0', [])                                                                                                                   
                                                                                                                                              
    # Previous P1 values are wiped.                                                                                                           
    rfm_parameter('P1', ['3', '4', '5'])                                                                                                      
                                                                                                                                              
                                                                                                                                              
@rfm.test                                                                                                                                     
class D(C):                                                                                                             
    '''                                                                                                                                       
    Change and extend the parameter space.                                                                                                    
    '''                                                                                                                                       
    # P0 no longer abstract                                                                                                                   
    rfm_parameter('P0', ['d'])                                                                                                                
                                                                                                                                              
    # P1 now has ['3', '4', '6']                                                                                                              
    rfm_parameter('P1', ['6'],                                                                                                                
                  inherit_params=True, # Extend the existing values for P1                                                                    
                  filt_params=lambda x: x[:-1] # Pop the last value of the inherited P1 values                                                
                 )                                                                                                                            
                                                                                                                                              
                                                                                                                                              
class E(D):                                                                                                             
    '''                                                                                                                                       
    Make P0 and P1 abstract parameters.                                                                                                      
    '''                                                                                                                                       
    rfm_purge_parameters('P0', 'P1')                                                                                                                
                                                                                                                                              
                                                                                                                                              
class F(D):                                                                                                             
    '''                                                                                                                                       
    Alternatively, one can also purge the whole parameter space without listing all the available parameters.
    '''                                                                                                                                       
    rfm_purge_all_parameters()   

Closes #1567

[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-01-20 10:32:07 UTC

[vkarak]

Some initial comments by reading the high-level description only:

1. I would prefer the directive to be named parameter() instead of rfm_parameter() and be exposed through the reframe module, so that users would write rfm.parameter() as they now write other rfm.* stuff.
2. What is the difference between rfm.test and rfm.simple_test? I find confusing to have both. Couldn't the rfm.simple_test take also on the role your rfm.test here?
3. In class C, it's not clear to me what makes it an abstract parameterized class. The fact that the parameter P0 is initialized as an empty list? That can be a bit counter intuitive, because somebody could simply set a parameter to an empty list for testing or a function that the user calls may return an empty list. I would vote for a more explicit way (if necessary) for marking that a parameter is abstract.
4. I would probably prefer the purge_parameter() to be an option of the parameter() directive instead.
5. Regarding the extension of the parameter space, now done with the inherit_params, I don't know if it would be possible or nicer to be achieved using language elements directly, e.g., rfm_parameter('P1', C.P1 + ['6']).

[jjotero]

I think it's better if we just focus on the high-level stuff for now :)

1. parameter() sounds good to me. Not sure there is the need to add the rfm.* to it before though - that's already done when you inherit from the rfm.RegressionTest.
2. With this new syntax, if we go for simple_test doing the test registration, one could write a parametrised test and decorate it with simple_test, which I would find a bit confusing. Moving forward, if the current way of writing parametrised tests is deprecated, there should be no need for two different decorators, and having something like test would be just enough.
3. P0 is not initialised as an empty list. For that you'd need parameter('P0', [[]]).
4. That is what something like parameter('P0', []) would do. The purpose of purge_parameters is just to give the user a shortcut to make multiple parameters abstract in a shorter way (purge_parameters('P0', 'P1', ..., 'Pn')).
5. I guess that could be done. But if you do this on, say class D in the example above and you do parameter('P0', A.P0 + ['4']), if one were to change class B and make it inherit from another class other than A, you'd be breaking all the classes upstream.

[vkarak]

1. I just mean that it should be exported at the reframe module, as we do with the RegressionTest etc., although they are defined in more esoteric modules.
2. If we go with test now, then practically we would need to either deprecate both simple_test and parameterized_test or have three decorators. Neither is very good I think.
3. Why do you need a list of lists? Also, wouldn't parameter('P0') be more natural as an "abstract parameter?"
4. Oh, so purge_parameters() purges the parameter space and makes them abstract. I thought it was just removing them. What if you were redefining them without any values, e.g.,parameter('P0'). Then you loose the convenience function, but that's not a big deal for the moment, I think.
5. I think we can leave it as of now.

[jjotero]

1. I'm not sure I follow 😅. In order to write rfm.parameter and so on, we'd simply need to wrap all these directives into a class and instantiate it as rfm in the RegressionTest namespace. Is that what you mean?
2. There isn't really a need to deprecate parameterized_test. That does not interfere at all with the new syntax and users could keep using that decorator if they want. This would be more of a renaming of simple_test into just test. But I guess that could stay as simple test for now.
3. I don't need lists (good point 😂). Yeah, agreed.
4. Yes, if we go for parameter('P0') to make a parameter abstract, then purge_parameters() would be a very minor shortcut to make multiple parameters abstract. I think it makes more sense to start without purge_parameters and we could include it later if we see it could be useful.
5. 👍

[vkarak]

1. No, I mean something simpler. Check how we even expose the RegressionTest under the reframe module. It will not require you to prefix the directive with rfm., as you are not required to write rfm.RegressionTest if you do from reframe import RegressionTest. I am just saying that we should do the same here. I need to look into the code, so ignore this for the moment. It's really just cosmetic.
2. Yes, let's stay with the not-so-simple_test-anymore for the moment :-)
3. Nice.
4. Let's go with parameter('P0') syntax and leave the purge shortcut for the future.

I will start looking into the implementation now for more details.

[jjotero]

Sounds good to me. I'll push some of these changes today.

[jjotero]

The unit tests that check the parameter space consumption with the decorators are still to do.

[vkarak]

Looks great! I haven't pressed the "Approve" button yet, because we need to think a bit about some forward-looking parts of the API and the finalization of the unit tests:

1. Perhaps we need to allow a type argument for the parameter, to be in sync with the future var() directive.
2. The public access to the parameter space could be done from something called params instead of param_space. Again, this is forward looking. Also, technically, the access is not read-only, since the users can still mess with the dictionary behind the scenes, since only the handle is read-only. We can consider returning a reframe.utility.MappingView there instead.

[jjotero]

1. What does var() do and how is it different from a parameter?
2. I think renaming param_space to params would add some confusion to the naming we use. When we register the tests we iterate over the parameter space and not over the parameters, therefore, the decorators do

def simple_test(cls):
    ...
    for _ in cls.parameter_space:
        _register_test(cls)

Here cls could have 2 parameter with 2 different values for each parameter. Therefore, your parameter space would have a length of 4. Then, the above loop would do 4 iterations if you do for _ in cls.params even though you've got only 2 parameters. That doesn't sound right to me. The users can access the parameters directly with cls.param_space.params in any case.

I'm adding a third item here :)
3. When writing the unit tests, I just noticed that the examples provided above by @victorusu would not work as things stand right now. The ParamSpace class has a unique_iter implemented which ensures that you only walk through the full parameter space once. Thus, when you decorate the class with parameterized_test, you would need to iterate over the full parameter space as many times as parameter sets are provided by the decorator. I see two solutions here:

• Allow for walking through the parameter space more than once (which I don't think it makes much sense in the context of how the parameters are defined and I also believe this opens a way for errors)
• Or we simply make incompatible to use the parameterized_test decorator with the new parameters. There cases shown above by @victorusu don't need to be decorated with the parameterized_test and they could simply add these parameters with the new method.

@rfm.simple_test
class SpackInstallSupportedMCApps(SpackInstallDaintMCApps):
    parameter('specs', 'gromacs@02020.4', 'gromacs@02020.2')
    parameter('options', '~cuda')
    parameter(variant, '', '+plumed', '+craypat')

    def __init__(self):
      ...
      self.valid_systems = ['daint:mc','dom:mc']
      self.valid_prog_environs = ['builtin']
      ...

[vkarak]

Hi @jjotero,

What does var() do and how is it different from a parameter?

This is to help users define new variables in their library tests and attach attributes to them (for example, types, whether it is required or not etc.) Check an older suggestion here: https://gist.github.com/vkarak/be9e88fa3c86abd50bfaac04a97d5fa5#file-gromacs-py-L20-L22

I think renaming param_space to params would add some confusion to the naming we use. When we register the tests we iterate over the parameter space and not over the parameters, therefore, the decorators do

For what you do, what you say makes perfectly sense. I'm trying to correlate this to the bigger picture of a test in the future. Again from that gist the idea was that the users could use setvar() with a when parameter to quickly slice the parameter space. When writing this as a user, I would like to write something compact, not parameter_space.

For your third point, I think the best idea is to make parameter() and @parameterized_test incompatible. I don't see a case that you would need both or that you couldn't achieve something with parameter() that you could with @paramterized_test, do you?

[jjotero]

OK, I now understand why there is a need for a different mechanism to set test variables that may depend on the parameter space and other factors. There are still a few things that confuse me on that gist, but let's not get into that yet, since it's independent from what's being done on this PR (perhaps move the discussion over to #1568?).

What would be needed for the setvar method mentioned on the previous comment, is a different structure that holds only the actual values that the parameters take on each instance of the test. No need for the full parameter space there, which is what self.param_space is. Therefore, that self.params could be added together with the setvar method at a later stage. Though, I'm not very convinced by the slicing of self.params suggested on the gist. That would fix the position of each parameter and that could become unmanageable for large parameter spaces. I have the feeling that using a dict there instead would simplify things.

Agree on the third point. I'll make them incompatible.

[vkarak]

The gist is certainly under discussion and we can revise it. It was a very first idea of how the tests might look like in the future. I just brought it in order to keep this in context, too.

[vkarak]

Just a minor comment.

[vkarak]

Latm. I will open a separate issue for updating the documentation and perhaps adding a tutorial about it ;-)

[vkarak]

I have not merged this yet, because I need to see how it shows up the documentation that you have put already.

[jjotero]

I've extended the directives section a bit and expanded the example. Even though this example uses the parameter directive, everything mentioned in there should also apply to the var directive. Also, the description of the parameter function is now more oriented towards its functionality rather than its implementation.

The parameter directive also shows as a member function of the RegressionTest class in the docs. I've included a link to the Directives section in there, and made clear that such function should only be called directly in the class body.

[vkarak]

Looks good, just minor comments and corrections.

[jjotero]

@jenkins-cscs retry all

[vkarak]

lgtm

[vkarak]

Goes in finally! 🍾 🎉