Skip to content

Conversation

@jjotero
Copy link
Contributor

@jjotero jjotero commented Feb 25, 2021

Rather than customising the test attribute access using the __getattribute__ method, this PR moves this customisation into the __getattr__ method instead, which is called when the default __getattribute__ raises an AttributeError.

This PR also makes the variable and parameter spaces behave as proper namespaces, which makes operations such as testing if a variable has been declared and so on more intuitive:

my_var_name in self._rfm_var_space

instead of

my_var_name in self._rfm_var_space.vars

However, adding new items into the variable or parameter spaces is disallowed:

class Foo(rfm.RegressionTest):
    pass

Foo.param_space['P0'] = (1,2,) # This raises an error
Foo._rfm_var_space['V0'] = 'a' # and this too

The corresponding unit tests have been added.

Closes #1841.

@jjotero jjotero added this to the ReFrame 3.5.0 milestone Feb 25, 2021
@jjotero jjotero requested review from ekouts and vkarak February 25, 2021 18:14
@jjotero jjotero self-assigned this Feb 25, 2021
@pep8speaks
Copy link

pep8speaks commented Feb 25, 2021

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-03-16 20:26:19 UTC

@vkarak vkarak removed this from the ReFrame 3.5.0 milestone Feb 25, 2021
@jjotero jjotero marked this pull request as draft March 4, 2021 17:55
@vkarak vkarak added this to the ReFrame sprint 21.03.1 milestone Mar 8, 2021
@jjotero jjotero changed the title [feat] Improve test attribute access [feat] Improve access to the namespaces of both parameter and variable built-ins Mar 9, 2021
@jjotero jjotero marked this pull request as ready for review March 9, 2021 15:47
@codecov-io
Copy link

Codecov Report

Merging #1819 (e95be91) into master (a3d0b0c) will decrease coverage by 1.52%.
The diff coverage is 43.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1819      +/-   ##
==========================================
- Coverage   87.61%   86.08%   -1.53%     
==========================================
  Files          49       49              
  Lines        8136     8378     +242     
==========================================
+ Hits         7128     7212      +84     
- Misses       1008     1166     +158     
Impacted Files Coverage Δ
reframe/core/variables.py 51.21% <38.37%> (-45.26%) ⬇️
reframe/core/meta.py 100.00% <100.00%> (ø)
reframe/core/namespaces.py 93.33% <100.00%> (+0.22%) ⬆️
reframe/core/parameters.py 98.50% <100.00%> (+0.04%) ⬆️
reframe/core/pipeline.py 91.86% <100.00%> (-0.05%) ⬇️

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 a3d0b0c...e95be91. Read the comment docs.

@jjotero
Copy link
Contributor Author

jjotero commented Mar 9, 2021

Variables now behave like regular python class attributes and multiple modifications within the same class body are now allowed. For example:

class Foo:
    pass

class MyTest(rfm.RegressionTest):
    v = variable(Foo, value=Foo())
    v.var_attribute = 'my attribute'

    # Override a regular attribute with a variable
    a = 4
    a = variable(int, value=40)
    print(a+1) # Prints 41

    # Modify an existing var
    a *= 10 # variable a is now 400

    # Initialize a variable as a function of another variable
    b = variable(float, value=a/3.141592)
    

    def __init__(self):
        print(self.v.var_attribute) # This prints 'my attribute'
  • Parameters and variables may override regular class attributes
  • A parameter may only be defined once per class body and it's value may not be altered after it was declared.
  • A variable's value may be modified multiple times within the same class body.
  • Overriding a variable with a parameter or the other way around results in undefined behaviour.

@jjotero jjotero requested a review from jgphpc March 9, 2021 17:34
@vkarak
Copy link
Contributor

vkarak commented Mar 12, 2021

Also coverage is a bit unhappy with the fact that none of the special functions are tested. I would add a small test test exercising all operators.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem that I see, which would help a lot in eliminating the need for a defining a constructor is that currently you can't use in an expression a built-in that was defined in a base class. Look at this?

class Base(rfm.RegressionTest):
    foo = variable(str, value='hello')


@rfm.simple_test
class HelloTest2(Base):
    descr = 'C Hello World test ' + foo
    valid_systems = ['*']
    valid_prog_environs = ['*']
    sourcepath = 'hello.c'
    tags = {'foo', 'bar'}
    sanity_patterns = sn.assert_found(r'Hello, World\!', 'foo.txt')
    maintainers = ['VK']

This will complain about foo in the HelloTest2 definition. I am wondering whether this ca be fixed or not?

Copy link
Contributor Author

@jjotero jjotero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few more unit tests covering the variable operators and the other issues mentioned in the PR review.

Copy link
Contributor Author

@jjotero jjotero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem that I see, which would help a lot in eliminating the need for a defining a constructor is that currently you can't use in an expression a built-in that was defined in a base class.

This is standard Python behaviour, isn't it? I have a few ideas to extend this behaviour. Let me do some "exploring" :)

@vkarak
Copy link
Contributor

vkarak commented Mar 15, 2021

This is standard Python behaviour, isn't it? I have a few ideas to extend this behaviour. Let me do some "exploring" :)

Yes, looks like that. It seems that a NameError is thrown before even looking it up in the MetaNamespace. The only reason I want this is to be able to define sanity_patterns in the class body. If you give a filename and not stdout, it just works with the current machinery. Technically, also, if you need something from the test instance, like the self.stdout, you should use a pipeline hook, but sanity patterns with stdout are so common and it would be nice to be able to define them in the class body. I am experimenting with a solution just for this. I wouldn't go too far to avoid this NameError (there is enough magic in the metaclasses already), I'm fine with that, as soon as I can put the damn sanity_patterns inside the class body.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, I have only a couple of minor style comments.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vkarak vkarak merged commit 1c9b188 into reframe-hpc:master Mar 16, 2021
@jjotero jjotero deleted the feature/improve-error-handling branch March 17, 2021 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make built-in variables behave like normal class attributes

5 participants