8000 Move run operations from `testcase` to `run/serial` by altheaden · Pull Request #428 · MPAS-Dev/compass · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Move run operations from testcase to run/serial #428

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 30, 2022
Merged

Move run operations from testcase to run/serial #428

merged 8 commits into from
Aug 30, 2022

Conversation

altheaden
Copy link
Collaborator

I moved all run operations from testcase.run() and various other internal methods to run/serial. This will allow future task parallel modifications to be limited to the run module. The run() method in testcase will be deprecated in favor of using step.runtime_setup() for custom setup operations at runtime.

I have tested this change by running the nightly suite against the baseline without any issues. I'll be making some changes to the documentation as well to account for the changes to the framework.

@altheaden altheaden added in progress This PR is not ready for review or merging framework labels Jul 28, 2022
@xylar xylar self-requested a review July 28, 2022 19:08
@xylar xylar self-assigned this Jul 28, 2022
@xylar
Copy link
Collaborator
xylar commented Aug 12, 2022

@altheaden, this is wonderful! The ocean tests all pass and are bit-for-bit with the previous version.

I'll take a more careful look at the documentation soon, but probably not today. It will be easier to review once #413 goes in.

@xylar
Copy link
Collaborator
xylar commented Aug 16, 2022

@altheaden, you can rebase this as soon as you're ready.

xylar
xylar reviewed Aug 18, 2022
View reviewed changes
Copy link
Collaborator
@xylar xylar left a comment

Choose a reason for hiding this comment

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

A few suggestions on the documentation. It will be easier to do a full review of the docs after a rebase.

@xylar xylar force-pushed the move_run_to_serial branch from fef3ade to 6a07053 Compare August 30, 2022 10:58
@xylar
Copy link
Collaborator
xylar commented Aug 30, 2022

@altheaden, I just rebased this branch. I tried to coordinate on Slack but I think you're busy with school having started last week for you. I just wanted to let you know that I'm going to move forward with this PR.

@xylar xylar removed the in progress This PR is not ready for review or merging label Aug 30, 2022
@xylar xylar marked this pull request as ready for review August 30, 2022 11:00
@xylar xylar force-pushed the move_run_to_serial branch from 56bc439 to 9cf9145 Compare August 30, 2022 11:26
@altheaden @xylar
Update documentation to Slurm terminology
2f5c23d 
Including usage of runtime_setup() and constrain_resources()
@xylar xylar force-pushed the move_run_to_serial branch 2 times, most recently from 5ff597b to b3ebaf0 Compare August 30, 2022 12:47
xylar added 3 commits August 30, 2022 15:01
@xylar
Add deprecation warning for test_case.run()
406937c 
Remove the deprecation warning from TestCase.run() because we
actually do want ot run the (do-nothing) default implementaiton.
@xylar
Update API docs
e792c82
@xylar
Further clarify runtime_setup() method in docs
880ccc8
@xylar xylar force-pushed the move_run_to_serial branch from b3ebaf0 to 880ccc8 Compare August 30, 2022 13:02
@xylar
Copy link
Collaborator
xylar commented Aug 30, 2022

Testing

I ran the ocean pr and landice full_ittegration test suites on Anvil. I verified that results for pr with Intel and Intel-MPI, and for full_integration with Gnu and MVAPICH were bit-for-bit with compass master.

I also verified that the expected warnings appeared indicating deprecated overriding of TestCase.run(). For example:

WARNING: Overriding the TestCase.run() method is deprecated.
  Please move the contents of
  compass.ocean.tests.global_convergence.cosine_bell.CosineBell.run()
  to the runtime_setup() or constrain_resources() methods of its steps.

@xylar
Copy link
Collaborator
xylar commented Aug 30, 2022 •
edited
Loading

@matthewhoffman and @trhille, I'm not going to ask you to review this. I believe I've done enough testing to be sure that it does no harm to landice. I just wanted to make you aware of this reorganization so it doesn't catch you by surprise.

It doesn't appear that any test cases in full_integration are overriding TestCase.run(), which is helpful!

xylar
xylar approved these changes Aug 30, 2022
View reviewed changes
Copy link
Collaborator
@xylar xylar left a comment

Choose a reason for hiding this comment

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

@altheaden, wonderful job!

I did a few final steps:

  • rebased
  • added the deprecation warning the way we discussed a couple of weeks ago
  • fixed a few things in the documentation

Other than the deprecation warning itself (my fault!), everything worked perfectly on my first try!

@xylar xylar merged commit 6842269 into MPAS-Dev:master Aug 30, 2022
@xylar xylar deleted the move_run_to_serial branch August 30, 2022 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xylar xylar xylar approved these changes

Assignees

@xylar xylar

Labels
framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@altheaden @xylar
0