Skip to content

Conversation

@jjotero
Copy link
Contributor

@jjotero jjotero commented Jul 1, 2021

This PR fixes a hook attachment leak occurring when a given hook from a base class is overridden by a derived class.

When overriding a pipeline hook, the method in the derived class would still get attached to the pipeline despite that it is not decorated to do so. See the example below:

@rfm.simple_test
class Base(rfm.RunOnlyRegressionTest):
    valid_systems = ['*']
    valid_prog_environs = ['*']
    executable = 'echo bananas'

    @sanity_function
    def assert_bananas(self):
        return sn.assert_found(r'bananas', self.stdout)

    @run_after('setup')
    def hh(self):
        print('bananas')

@rfm.simple_test
class Derived(Base):
    def hh(self):
        print('not bananas')

Running this gives:

[==========] Running 2 check(s)
[==========] Started on Thu Jul  1 13:06:40 2021 

[----------] started processing Base (Base)
[ RUN      ] Base on dom:login using PrgEnv-gnu
bananas
[----------] finished processing Base (Base)

[----------] started processing Derived (Derived)
[ RUN      ] Derived on dom:login using PrgEnv-gnu
not bananas
[----------] finished processing Derived (Derived)

[----------] waiting for spawned checks to finish
[       OK ] (1/2) Base on dom:login using PrgEnv-gnu [compile: 0.006s run: 1.239s total: 1.266s]
[       OK ] (2/2) Derived on dom:login using PrgEnv-gnu [compile: 0.014s run: 1.316s total: 1.349s]
[----------] all spawned checks have finished

[  PASSED  ] Ran 2/2 test case(s) from 2 check(s) (0 failure(s), 0 skipped)
[==========] Finished on Thu Jul  1 13:06:42 2021 

The method Derived.hh still gets attached to the post_setup stage, despite that this method has not even been decorated with any run_before or run _after decorators.

@jjotero jjotero added this to the ReFrame Sprint 21.06.2 milestone Jul 1, 2021
@jjotero jjotero requested review from ekouts, victorusu and vkarak July 1, 2021 11:27
@jjotero jjotero self-assigned this Jul 1, 2021
Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

@jjotero, I think we need a unit test for this PR.
This behavior should be guaranteed to not break.
What do you think about adding your example as a unit test?
And also another unit test that re-attaches the hook?

@vkarak vkarak changed the title [bugfix] Hook override [bugfix] Fix pipeline hook override Jul 2, 2021
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 some minor comments + I agree with @victorusu to provide a unit test.

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2021

Codecov Report

Merging #2048 (148b16c) into master (2b2db4e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2048      +/-   ##
==========================================
- Coverage   87.53%   87.51%   -0.02%     
==========================================
  Files          50       50              
  Lines        8889     8891       +2     
==========================================
  Hits         7781     7781              
- Misses       1108     1110       +2     
Impacted Files Coverage Δ
reframe/core/hooks.py 90.90% <100.00%> (-1.88%) ⬇️
reframe/core/meta.py 98.96% <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 2b2db4e...148b16c. Read the comment docs.

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, just a very minor comment.

@vkarak vkarak requested a review from victorusu July 5, 2021 13:47
Copy link
Contributor

@victorusu victorusu 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 265104b into reframe-hpc:master Jul 7, 2021
@jjotero jjotero deleted the bugfix/hook-override branch July 7, 2021 09:08
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.

5 participants