Skip to content

Conversation

@ekouts
Copy link
Contributor

@ekouts ekouts commented Jul 1, 2021

Closes #1797.
I liked a bit more the argument option, than the built-in syntax, but we can change it. The new syntax is:

class BaseFoo(rfm.RegressionTest, require_version=['x.x.x', '>y.y.y']):
   ...

@rfm.simple_test
class Foo(BaseFoo):
   ...

Foo and all descendants will inherit the required versions, unless they specify require_version again.

@ekouts ekouts added this to the ReFrame Sprint 21.06.2 milestone Jul 1, 2021
@ekouts ekouts requested review from jjotero and vkarak July 1, 2021 06:57
@ekouts ekouts self-assigned this Jul 1, 2021
@ekouts ekouts marked this pull request as draft July 1, 2021 06:57
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2021

Codecov Report

Merging #2046 (b3c4697) into master (4f3d906) will decrease coverage by 0.01%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2046      +/-   ##
==========================================
- Coverage   86.76%   86.74%   -0.02%     
==========================================
  Files          52       52              
  Lines        9261     9271      +10     
==========================================
+ Hits         8035     8042       +7     
- Misses       1226     1229       +3     
Impacted Files Coverage Δ
reframe/core/decorators.py 64.81% <66.66%> (+0.10%) ⬆️
reframe/core/pipeline.py 91.40% <80.00%> (-0.12%) ⬇️

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 4f3d906...b3c4697. Read the comment docs.

Copy link
Contributor

@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.

Just a small comment on the warning message. The rest looks good.

@pep8speaks
Copy link

pep8speaks commented Jul 2, 2021

Hello @ekouts, 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-07-14 20:02:07 UTC

@ekouts ekouts marked this pull request as ready for review July 2, 2021 07:24
Copy link
Contributor

@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.

We need to document these arguments in the manuals. I think that the best place is by adding the required_version (and pin_prefix and special) in the docstring of the RegressionTest base class.

@jjotero
Copy link
Contributor

jjotero commented Jul 7, 2021

Another thing that just crossed my mind is that we should probably be consistent with how we name the class arguments. For example, pin_prefix indicates an action and required_version does not. Changing this arg to require_version would make this naming consistent, I think.

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.

I agree with @jjotero that we should find a proper location to document the class arguments. The natural place would be when documenting the RegressionTest class, but I don't know how we can inject in the documentation the arguments of the __init_subclass__.

@ekouts
Copy link
Contributor Author

ekouts commented Jul 13, 2021

The documentation of the parameters is not ideal. Besides the signature of the RegressionTest, the documentation of the parameters doesn't appear exactly right.
Screenshot 2021-07-13 at 08 32 05
The second format of require_version has some grey background and the description ("is a list of ReFrame version...") starts from the next line

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.

I have a couple of minor comments. I will have a look at the formatting problem you mentioned and I'll try to fix them too.

@vkarak vkarak requested a review from jjotero July 13, 2021 20:57
@vkarak vkarak changed the title [feat] Make the @required_version decorator a class argument [feat] Make the @required_version decorator a class definition parameter Jul 14, 2021
@vkarak vkarak merged commit 285d22f into reframe-hpc:master Jul 14, 2021
@ekouts ekouts deleted the feat/required_version branch September 3, 2021 13:51
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 the @required_version decorator a built-in

5 participants