Skip to content

Conversation

@GiuseppeLoRe
Copy link
Contributor

@GiuseppeLoRe GiuseppeLoRe commented Sep 13, 2019

No description provided.

@pep8speaks
Copy link

pep8speaks commented Sep 13, 2019

Hello @GiuseppeLoRe, 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 2019-09-16 14:49:27 UTC

@jenkins-cscs
Copy link
Collaborator

Can I test this patch?

@teojgo
Copy link
Contributor

teojgo commented Sep 13, 2019

ok to test

@teojgo teojgo requested review from teojgo and vkarak September 13, 2019 13:11
@teojgo teojgo changed the title Added daint to the systems where to run the IOR test, both on gpfs an… [test] Added daint to the systems where to run the IOR test, both on gpfs an… Sep 13, 2019
Copy link
Contributor

@teojgo teojgo left a comment

Choose a reason for hiding this comment

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

You can remove the super().__init__() call in line 10. It is not needed anymore. Furthermore, remove snx1600 from the parameterized tests since you removed it from the filesystems you check.

@codecov-io
Copy link

codecov-io commented Sep 13, 2019

Codecov Report

Merging #937 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #937   +/-   ##
======================================
  Coverage    91.7%   91.7%           
======================================
  Files          79      79           
  Lines       10486   10486           
======================================
  Hits         9616    9616           
  Misses        870     870

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 7024c0a...262e4c1. Read the comment docs.

@vkarak vkarak changed the title [test] Added daint to the systems where to run the IOR test, both on gpfs an… [test] Enable IOR test on Daint Sep 14, 2019
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 self-requested a review September 15, 2019 09:12
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.

Have you tried that on Daint? For me, some tests fail. Also I think that the numbers reported by one of the read tests are skewed due to caching.

@GiuseppeLoRe
Copy link
Contributor Author

Without the line
super().init() in IorCheck.init
I get this error

AttributeError: IorWriteCheck object has no attribute '_current_partition'

@teojgo
Copy link
Contributor

teojgo commented Sep 16, 2019

@GiuseppeLoRe You can remove the super().__init()__ only in the base class that inherits the rfm.RegressionTest.

@vkarak
Copy link
Contributor

vkarak commented Sep 16, 2019

@teojgo Perhaps @GiuseppeLoRe uses an older version of reframe. This feature was introduced in 2.18 2.19.

@GiuseppeLoRe
Copy link
Contributor Author

I am using the daint default one, 2.17.1

@GiuseppeLoRe
Copy link
Contributor Author

Should I test with 2.18 or 2.19?

@vkarak
Copy link
Contributor

vkarak commented Sep 16, 2019

So then keep the call to __init__(), because we are going to use this test on Daint immediately.

@GiuseppeLoRe
Copy link
Contributor Author

ok

@GiuseppeLoRe
Copy link
Contributor Author

Now that I do the tests twice, one for the gpu partition and then again for the mc one, I get the caching effect on the second test.
Is there a variable "current_partition" I can use to differentiate the output files?

@vkarak
Copy link
Contributor

vkarak commented Sep 16, 2019

@GiuseppeLoRe Yes, you can. This information is available in the setup() method, so you will have to override this in your test. Check the example here on how to do this. The link refers to reframe 2.17, which is the version you are using.

@GiuseppeLoRe
Copy link
Contributor Author

@vkarak thanks for the link, but at least for me the example is not very clear.

@vkarak
Copy link
Contributor

vkarak commented Sep 16, 2019

@GiuseppeLoRe What do you want to do based on the current programming environment?

@GiuseppeLoRe
Copy link
Contributor Author

@vkarak I would like the partition mane (mc or gpu) to build the ior output file name. And thought that I could use something like https://reframe-hpc.readthedocs.io/en/stable/reference.html#reframe.core.pipeline.RegressionTest.current_partition....

@vkarak
Copy link
Contributor

vkarak commented Sep 16, 2019

Yes, you can do that, but current_partition is only defined after the setup stage of the pipeline. You can do it as follows:

    def setup(self, partition, environ, **job_opts):
        super().setup(partition, environ, **job_opts)
        if self.current_partition.name == 'gpu':
            self.executable_opts = [...]
        else:
            self.executable_opts = [...]

You may also write this as follows:

    def setup(self, partition, environ, **job_opts):
        if partition.name == 'gpu':
            self.executable_opts = [...]
        else:
            self.executable_opts = [...]

        super().setup(partition, environ, **job_opts)

And from version 2.20 (or even from the current master), you will be able to write this as follows:

    @rfm.run_after('setup')
    def choose_your_function_name(self):
        if self.current_partition.name == 'gpu':
            self.executable_opts = [...]
        else:
            self.executable_opts = [...]    

…e gpu partition are different from the files in the mc one. This to avoid caching effects in the read tests.
@GiuseppeLoRe GiuseppeLoRe requested a review from vkarak September 16, 2019 14:54
@GiuseppeLoRe
Copy link
Contributor Author

Tested on Daint.

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.

Overall lgtm now. My only question is whether it would make sense to run the test for /users from the login nodes rather than the compute nodes, so as to avoid the wait times.

@vkarak
Copy link
Contributor

vkarak commented Sep 17, 2019

@GiuseppeLoRe The test is MPI-based, so ignore my comment. I am merging this.

@vkarak vkarak merged commit 66517f4 into reframe-hpc:master Sep 17, 2019
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.

6 participants