Skip to content

[production/AQM.v7] Remove extrn_ic/lbcs and nexus_gfs_sfc#955

Merged
chan-hoo merged 7 commits into
ufs-community:production/AQM.v7from
chan-hoo:feature/rm_extrn
Oct 27, 2023
Merged

[production/AQM.v7] Remove extrn_ic/lbcs and nexus_gfs_sfc#955
chan-hoo merged 7 commits into
ufs-community:production/AQM.v7from
chan-hoo:feature/rm_extrn

Conversation

@chan-hoo
Copy link
Copy Markdown
Collaborator

@chan-hoo chan-hoo commented Oct 27, 2023

DESCRIPTION OF CHANGES:

  • Move the get_extrn_ics, get_extrn_lbcs, and nexus_gfs_sfc tasks into their subsequent tasks make_ics, make_lbcs, and nexus_emission.
  • Remove the j-job and ex-scripts for the above three unnecessary tasks.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

TESTS CONDUCTED:

  • hera.intel
  • orion.intel
  • hercules.intel
  • cheyenne.intel
  • cheyenne.gnu
  • derecho.intel
  • gaea.intel
  • gaeac5.intel
  • jet.intel
  • wcoss2.intel
  • NOAA Cloud (indicate which platform)
  • Jenkins
  • fundamental test suite
  • comprehensive tests (specify which if a subset was used)

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@JianpingHuang-NOAA, Please test this PR "thoroughly" unlike the previous PR. I asked you to test the previous PR with your NRT run and you said it worked well. However, the previous PR screwed up the rocoto part. I don't understand how your test was passed successfully. I had to spend some time to fix it in this PR.

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

chan-hoo commented Oct 27, 2023

@JianpingHuang-NOAA, @rmontuoro, @MatthewPyle-NOAA, @HaixiaLiu-NOAA, @aerorahul, @lgannoaa, Based on the NCO standards (page 13, viii) (e.g. a forecast job writing output that is needed by a post job running in parallel), I restore this capability (running run_fcst and run_post in parallel) for AQM in this PR. It was initially designed to work for SRW as well as AQM, but I was requested to remove this capability for AQM some time ago. This caused some confusion regarding the unique_id issue. I think this capability is allowed by the NCO standards. Therefore, I think the unique_id issue in AQM will be resolved by this PR (at least in the "rocoto" perspective).

@JianpingHuang-NOAA
Copy link
Copy Markdown

@chan-hoo Two questions 1) Which PR do you refer ? If you refer to the latest PR that Lin opened, I was told by Lin that is for the operational implementation and we are not allowed to test it directly without any change. If you change some change for rocoto's use, that will cause a trouble to ecflow. Please check with Lin. 2) Does the PR 955 include the changes that we discussed yesterday? @lgannoaa

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@JianpingHuang-NOAA, understood. However, we should test this PR with rocoto first. Then, we can ask @lgannoaa to open a new PR for ecflow because we are not familiar with ecflow. Please test this PR with your NRT run. If it works, we can ask Lin for a next PR for ecflow.

@lgannoaa
Copy link
Copy Markdown

@JianpingHuang-NOAA, @rmontuoro, @HaixiaLiu-NOAA, @aerorahul, @lgannoaa, Based on the NCO standards (page 13, viii) (e.g. a forecast job writing output that is needed by a post job running in parallel), I restore this capability (running run_fcst and run_post in parallel) for AQM in this PR. It was initially designed to work for SRW as well as AQM, but I was requested to remove this capability for AQM some time ago. This caused some confusion regarding the unique_id issue. I think this capability is allowed by the NCO standards. Therefore, I think the unique_id issue in AQM will be resolved by this PR.

@chan-hoo please only proceed with the work that we have agreed yesterday. There is no agreement for you to modify forecast and post at this time.

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@JianpingHuang-NOAA, regarding your second question, yes, this PR removes get_extrn_ics/lbcs and nexus_gfs_sfc and adds these tasks into their subsequent tasks.

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@lgannoaa, you don't have to worry about it. it doesn't impact the ecflow run at all. I make this capability work only when WORKFLOW_MANAGER="rocoto".

@JianpingHuang-NOAA
Copy link
Copy Markdown

@chan-hoo Ye, I did test Lin's PR by modifying the following 5 ex-scripts on my local test version on WCOSS2 (/lfs/h2/emc/physics/noscrub/jianping.huang/nwdev/packages/aqm.v7.0.86a)

  1. ~/script/
    exregional_aqm_ics.sh_rocoto
    exregional_make_lbcs.sh_rocoto
    exregional_run_post.sh_rocoto*
    exregional_make_ics.sh_rocoto
    exregional_nexus_emission.sh_rocoto

  2. ~/ush/load_modules_run_task.sh

but I did not commit these changes into the Lin's branch to avoid any trouble. This may cause some confusion to you.

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@JianpingHuang-NOAA, totally understood now. I am sorry for my misunderstanding regarding the previous PR. Please test this PR with your NRT run.

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@JianpingHuang-NOAA, when you test this PR, please don't forget to update your config.yaml as follows:

workflow_switches:
  RUN_TASK_GET_EXTRN_ICS: false
  RUN_TASK_GET_EXTRN_LBCS: false
  RUN_TASK_NEXUS_GFS_SFC: false

@rmontuoro
Copy link
Copy Markdown

@lgannoaa, you don't have to worry about it. it doesn't impact the ecflow run at all. I make this capability work only when WORKFLOW_MANAGER="rocoto".

@chan-hoo, @JianpingHuang-NOAA, and @lgannoaa - Please limit the code changes to what discussed yesterday. Changes to the forecast and post jobs are not required. Any change at this stage (even if only for rocoto) introduces additional risk and may have unintended consequences.

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@rmontuoro, the change for run_post has been removed.

@JianpingHuang-NOAA
Copy link
Copy Markdown

@chan-hoo I completed the test with your feature branch rm_extrn. The test results can be found on Dogwood at

/lfs/h2/emc/ptmp/jianping.huang/emc.para/com/aqm/v7.0/c86b.20231026/00

All the grib2 files are identical to the real-time test results with the old package before you made the changes. (/lfs/h2/emc/ptmp/jianping.huang/emc.para/com/aqm/v7.0/aqm.20231026/00)

So, the test is completed. Please go ahead to merge PR 995 into the branch production/AQM.v7 now. Thanks !

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@BenjaminBlake-NOAA, could you please approve this PR? Since I opened this PR, you are the only person who can approve this PR. :) :)

@rmontuoro
Copy link
Copy Markdown

@rmontuoro, the change for run_post has been removed.

Thank you, @chan-hoo. I appreciate your prompt attention and contributions to this update.

@JianpingHuang-NOAA
Copy link
Copy Markdown

@BenjaminBlake-NOAA, Can you review and approve the PR? I am waiting for your response. Thanks, !

Copy link
Copy Markdown
Collaborator

@BenjaminBlake-NOAA BenjaminBlake-NOAA left a comment

Choose a reason for hiding this comment

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

@chan-hoo @JianpingHuang-NOAA The changes look good to me. One quick question before I approve this PR: Why is the exregional_run_post.sh script modified? According to the above discussion I thought any changes to the run_post task were removed. Thanks

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@BenjaminBlake-NOAA, due to the change in the previous PR for ecflow, the 'rocoto' run doesn't work any more. The change in the run_post script makes both ecflow and rocoto run correctly.

Copy link
Copy Markdown
Collaborator

@BenjaminBlake-NOAA BenjaminBlake-NOAA left a comment

Choose a reason for hiding this comment

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

@chan-hoo Thanks for clarifying, approving now

@chan-hoo
Copy link
Copy Markdown
Collaborator Author

@BenjaminBlake-NOAA, Thank you so much! @JianpingHuang-NOAA @rmontuoro @lgannoaa, I'll merge this PR.

@chan-hoo chan-hoo merged commit e15ff43 into ufs-community:production/AQM.v7 Oct 27, 2023
@JianpingHuang-NOAA
Copy link
Copy Markdown

@chan-hoo Great. Thanks !

@chan-hoo chan-hoo deleted the feature/rm_extrn branch February 14, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants