Skip to content

Conversation

@rsarm
Copy link
Contributor

@rsarm rsarm commented Nov 15, 2018

Closes #510

@codecov-io
Copy link

codecov-io commented Nov 15, 2018

Codecov Report

Merging #583 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
+ Coverage   91.53%   91.56%   +0.02%     
==========================================
  Files          72       72              
  Lines        9005     9005              
==========================================
+ Hits         8243     8245       +2     
+ Misses        762      760       -2
Impacted Files Coverage Δ
reframe/core/config.py 84.34% <0%> (+1.73%) ⬆️

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 2879b7d...eed3cf6. 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.

As far as I remember, we have said that we should look into more variables. Not just $SCRATCH.

@victorusu @lucamar Can you please review as well?

@vkarak vkarak requested review from lucamar and removed request for teojgo November 15, 2018 15:30
@lucamar
Copy link
Contributor

lucamar commented Nov 19, 2018

Thanks @rsarm: is it possible to check also the items proposed in #524 ?

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.

I have just one comment to extend the test. Probably we do not need to do it now. But it would be nice to have an extended version of this test.

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.

Generating a separate test for each env variable seems too much. Why not checking all the variables from a single check? The overall overhead for running will be much lower. You may execute echo VAR=$VAR instead of $VAR and update the regex pattern. If it fails, again you will immediately have the offending variable name.

@vkarak
Copy link
Contributor

vkarak commented Dec 4, 2018

@rsarm The problem you are seeing is due to the fact that a regression test is deep copied for each combination of system partition and programming environment, but the closure of the lambda function still refers to the original object, because functions are not deep copied. More specifically, when you create the lambda function for the map(), its closure contains self. At the time the lambda is defined self refers to the object created during loading of the test (by the @simple_test decorator). Before executing the test, ReFrame clones it, but the self of its closure still refers to the original object, which is not executed. As a result, self._job is None. Here is a small reproducer that showcases the problem:

import copy


class C:
    def __init__(self):
        self.fn = lambda: self


if __name__ == '__main__':
    c = C()
    d = copy.deepcopy(c)
    print('c is bound to', c)
    print('d is bound to', d)
    print('c.fn() is bound to', c.fn())
    print('d.fn() is bound to', d.fn())

This will print the following, which demonstrates exactly the problem:

c is bound to <__main__.C object at 0x2b073e23e780>
d is bound to <__main__.C object at 0x2b073e267470>
c.fn() is bound to <__main__.C object at 0x2b073e23e780>
d.fn() is bound to <__main__.C object at 0x2b073e23e780>

A workaround to this bug is to avoid functions with closures. So you could rewrite your sanity function as follows:

    def __init__(self):
        ...
        self.sanity_patterns = sn.all(
            sn.map(self.assert_envvar, ['CRAY_CPU_TARGET=haswell']))

    def assert_envvar(self, v):
        return sn.assert_found(v, self.stdout)

@pep8speaks
Copy link

pep8speaks commented Dec 5, 2018

Hello @rsarm! Thanks for updating the PR.

Comment last updated on December 06, 2018 at 15:34 Hours UTC

@vkarak
Copy link
Contributor

vkarak commented Dec 7, 2018

@rsarm Can you revert your latest changes? I have fixed the PR myself yesterday, but forgot to merge it.

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.

@rsarm Please revert your last changes.

@vkarak vkarak merged commit c1a1612 into reframe-hpc:master Dec 7, 2018
@rsarm rsarm deleted the test/env-ssh branch November 26, 2019 13:16
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.

7 participants