Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make compass setup create a custom test suite #191

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Jul 30, 2021

This is convenient for running several test cases in one command even if they aren't part of a predefined test suite. Test cases are run in the order they are supplied to compass setup.

Some functionality has been moved from the suite module into setup and clean to facilitate setting up and cleaning up custom test suites in the same way as predefined ones.

A small bug has been fixed in determining the number of cores that a suite requires. Previously, all steps were used. With the fix, only steps_to_run are used.

The docs have been updated to include new custom suites.

@xylar xylar added enhancement New feature or request python package DEPRECATED: PRs and Issues involving the python package (master branch) labels Jul 30, 2021
@xylar xylar self-assigned this Jul 30, 2021
@xylar
Copy link
Collaborator Author

xylar commented Jul 30, 2021

Testing

With the fixes in #190, I was able to set up a "custom" test suite with all ocean test cases.

I also set up and successfully ran a custom suite with the 6 ISOMIP+ test cases.

@xylar
Copy link
Collaborator Author

xylar commented Jul 30, 2021

Rationale

I believe this will be a helpful step toward resolving an issue with validation that we're having in #164. We need a way to perform validation between outputs from different test cases, which is not currently supported. But we need a way to only perform that validation if both steps of both test cases are actually being run. The clearest way I can see to do this is to make sure both test case are run as part of a test suite.

I have also found it's pretty tedious to define a one-off test suite for temporary work but it's also tedious to cd into test cases or steps to run them one after another. I think this will generally be helpful.

Finally, it is nice to have the information about processor requirements that suites provide during setup and the timings and other summary information they print when they finish, neither of which is currently provided when test cases get set up and run outside of a suite.

@xylar
Copy link
Collaborator Author

xylar commented Jul 30, 2021

@matthewhoffman, @mark-petersen and @cbegeman, first please let me know if you think this is a good idea. Second, please give it a quick whirl when you can. It should be very quick to test. Let me know how it goes.

@xylar
Copy link
Collaborator Author

xylar commented Aug 2, 2021

I don't think this work is related to #164 after all (it isn't yet clear to me how to tell if a step within a test suite got run or not other than if the output exist, and the output from validation gets kind of buried when running a suite) but I think it will still be useful in general.

This is convenient for running several test cases in one command
even if they aren't part of a predefined test suite.  Test cases
are run in the order they are supplied to `compass setup`.

Some functionality has been moved from the ``suite`` module into
``setup`` and ``clean`` to facilitate setting up and cleaning up
custom test suites in the same way as predefined ones.

A small bug has been fixed in determining the number of cores
that a suite requires.  Previously, all steps were used.  With
the fix, only ``steps_to_run`` are used.
Copy link
Collaborator

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

Yes! This is a wonderful addition! I tried it with:

compass setup -p $PATH -w $WORKDIR -n 40 41 42
cd $WORKDIR
compass run  # (on a compute node)

and it works! It runs just those three cases! This is like getting our first microwave when I was ten - you didn't know you wanted it before, and now it's indispensable!

@xylar
Copy link
Collaborator Author

xylar commented Aug 5, 2021

@mark-petersen, thanks! I've been doing work in other branches and have been really tempted to rebase them onto this branch because it's really annoying not to have this feature. I'm glad you agree.

@mark-petersen
Copy link
Collaborator

@matthewhoffman and @cbegeman if you are comfortable approving based on our testing and comments, please go ahead. It would be nice to merge this in soon.

@cbegeman
Copy link
Collaborator

cbegeman commented Aug 5, 2021

Thanks for the reminder, @xylar. I tested it and it works fine for me as well. I personally don't find it cumbersome to add compass/ocean/suites/custom-suite.txt, but maybe I'm missing something. And I like how you've tucked the minimum cores determination in setup_cases. So I'm fine with you merging.

Copy link
Member

@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

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

I looked over the changes and don't see anything that concerns me. I can see how this can be a useful feature. Given the other testing reported by others, I'm comfortable with this being merged without me doing more.

@xylar
Copy link
Collaborator Author

xylar commented Aug 6, 2021

Thanks @cbegeman and @matthewhoffman for your reviews!

@xylar xylar merged commit efe4a37 into MPAS-Dev:master Aug 6, 2021
@xylar xylar deleted the setup_to_custom_suite branch August 6, 2021 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request python package DEPRECATED: PRs and Issues involving the python package (master branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants