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

Addition of python script to add yaml-based test option + Creation of FV3 diag table documentation #2277 + SKEB fix with d_con = zero #2374 #2278

Merged
merged 294 commits into from
Aug 12, 2024

Conversation

jkbk2004
Copy link
Collaborator

@jkbk2004 jkbk2004 commented May 13, 2024

Commit Queue Requirements:

  • Fill out all sections of this template.
  • All sub component pull requests have been reviewed by their code managers.
  • Run the full Intel+GNU RT suite (compared to current baselines) on either Hera/Derecho/Hercules
  • Commit 'test_changes.list' from previous step

Description:

Commit Message:

* UFSWM - python scripts for yaml and rocoto-xml conversion, experiment setup, and test log output  
* Add python superlint option
* Documentation update: doc/UsersGuide/source/tables/fv3_diag_table.rst
    * FV3 - 
    * atmos_cubed_sphere - bugfix: allocates heat_source when skeb is True and d_con is zero
 

Priority:

  • Normal

Git Tracking

UFSWM:

Sub component Pull Requests:

UFSWM Blocking Dependencies:

  • None

Changes

Regression Test Changes (Please commit test_changes.list):

  • No Baseline Changes.

Input data Changes:

  • None.

Library Changes/Upgrades:

  • No Updates

Testing Log:

  • RDHPCS
    • Hera
    • Orion
    • Hercules
    • Jet
    • Gaea
    • Derecho
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
  • opnReqTest (complete task if unnecessary)

@jkbk2004
Copy link
Collaborator Author

jkbk2004 commented Aug 9, 2024

@zach1221 can you confirm about -c option on your side ?

@zach1221
Copy link
Collaborator

Ok I ran ufs_test.sh on Hercules and it looks like after @jkbk2004's changes, the rocoto_workflow.xml file skips all of the dependent tests. For Example, running with "-c" command, tests cpld_control_p8_intel, and cpld_control_p8.v2.sfc_intel run then cpld_restart_p8_intel, cpld_control_qr_p8_intel, cpld_restart_qr_p8_intel, etc are skipped, until the next non-dependency test cpld_control_ciceC_p8_intel runs. So in short, dependent tests are being skipped when creating baselines.

@BrianCurtis-NOAA
Copy link
Collaborator

Ok I ran ufs_test.sh on Hercules and it looks like after @jkbk2004's changes, the rocoto_workflow.xml file skips all of the dependent tests. For Example, running with "-c" command, tests cpld_control_p8_intel, and cpld_control_p8.v2.sfc_intel run then cpld_restart_p8_intel, cpld_control_qr_p8_intel, cpld_restart_qr_p8_intel, etc are skipped, until the next non-dependency test cpld_control_ciceC_p8_intel runs. So in short, dependent tests are being skipped when creating baselines.

@DusanJovic-NOAA Could you verify that you see the same?

@DusanJovic-NOAA
Copy link
Collaborator

I see that the cpld_2threads_p8 test is now not executed when rt is running in baseline create mode (-c), which is good. But I also see that this commit 9c48b52 now specify that a lot more tests depend on their corresponding control test , which they do not depend on. I guess these 'dependency' flags are now used to skip baseline creation. Which is not correct. Like I explained, or tried to explain, in my previous comment (#2278 (comment) which for some reason is marked as 'resolved') 'baseline' and 'dependency' are two different attributes. Using just 'dependency' to describe both is not going to work in general. For example for tests that both require new baselines and depend on some other test, like:

RUN | cpld_control_gfsv17_iau                           | - noaacloud                          | baseline | cpld_control_gfsv17

or:

RUN | atm_ds2s_docn_dice                                | - noaacloud wcoss2 acorn             | baseline | cpld_control_nowave_noaero_p8

For these tests in baseline create mode (-c) baselines will not be created because they 'depend' on other test. Also adding these extra 'dependency' flag to tests that do not depend other test will unnecessarily slow down the overall execution of rt.

But, since I see that for some reason there is a pressure to merge this PR, I'm going to remove my 'request for change' which blocks the merge, but unfortunately I can not approve this PR.

@jkbk2004
Copy link
Collaborator Author

@DusanJovic-NOAA thanks for the comment! hopefully, full dependency and baseline creation features can be addressed when cpld_control_gfsv17_iau is turned on next round. performance tuning as well. @ulmononian FYI: additional test requirement.

@ulmononian
Copy link
Collaborator

@DusanJovic-NOAA thanks for the comment! hopefully, full dependency and baseline creation features can be addressed when cpld_control_gfsv17_iau is turned on next round. performance tuning as well. @ulmononian FYI: additional test requirement.

@jkbk2004 indeed -- noted.

@jkbk2004
Copy link
Collaborator Author

@BrianCurtis-NOAA can you close the request so that @FernandoAndrade-NOAA can move on for merging this pr?

@BrianCurtis-NOAA
Copy link
Collaborator

@jkbk2004 I would recommend creating an issue to address Dusan's concerns on the tests that need to create baselines even if they depend on another test so you can follow up accordingly as you progress with the development of your testing framework.

@jkbk2004
Copy link
Collaborator Author

Opened #2397

@FernandoAndrade-NOAA
Copy link
Collaborator

Ok, I'll get started with letting atmos cubed sphere to proceed with their merge.

@zach1221 zach1221 merged commit a1143cc into ufs-community:develop Aug 12, 2024
3 checks passed
@jkbk2004 jkbk2004 mentioned this pull request Aug 12, 2024
14 tasks
@jkbk2004 jkbk2004 deleted the feature/rt-refactor branch August 19, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EPIC Support Requested No Baseline Change No Baseline Change Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.