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

Add a job to prepare ioda format dumps for use in atmospheric UFS-DA #1826

Merged

Conversation

RussTreadon-NOAA
Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA commented Aug 28, 2023

Description

Add a job to convert bufr format GDA dumps to ioda format for use in atmospheric UFS-DA.

Resolves #1820
Depends on

Type of change

  • New feature (adds functionality)

This PR adds j-job JGLOBAL_PREP_IODA_OBS and rocoto driver prepiodaobs.sh to the suite of cycled gfs jobs. Changes are made in workflow scripts to generate the xml required to run prepioodaobs. The new job is added to config.resources along with the addition of a new config file, config.prepiodaobs.

At present GDASApp only converts bufr satwnd dumps to ioda format. Additional bufr dump types will be added in the future.

Change characteristics

  • Is this a breaking change (a change in existing functionality)? NO
  • Does this change require a documentation update? NO

How has this been tested?

  • Clone and build on Hera and Orion
  • Cycled test on Hera Run C384/C192 L127 for 2021081418.

Checklist

  • Any dependent changes have been merged and published
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code

Copy link
Contributor

@WalterKolczynski-NOAA WalterKolczynski-NOAA left a comment

Choose a reason for hiding this comment

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

I think we need more information about what the exscript is actually doing, especially with the $COM_OBS directory. In operations, that directory does NOT fall under GFS (it is an obsproc directory). If the script is placing output there (and I don't see any other COM directories being defined), that is probably a no-go. It might be okay temporarily for development, but we need to start thinking about making all this new JEDI stuff ops-ready.

jobs/rocoto/prepiodaobs.sh Outdated Show resolved Hide resolved
parm/config/gfs/config.resources Outdated Show resolved Hide resolved
@RussTreadon-NOAA
Copy link
Contributor Author

RussTreadon-NOAA commented Aug 31, 2023

I think we need more information about what the exscript is actually doing, especially with the $COM_OBS directory. In operations, that directory does NOT fall under GFS (it is an obsproc directory). If the script is placing output there (and I don't see any other COM directories being defined), that is probably a no-go. It might be okay temporarily for development, but we need to start thinking about making all this new JEDI stuff ops-ready.

A comment was added to issue #1820 briefly describing the exscript. GDASApp PR #575 provides additional details regarding bufr2ioda processing.

I believe you are correct. The bufr to ioda processing in this PR is a temporary step for UFS-DA prototyping. Please contact @CoryMartin-NOAA, @emilyhcliu, and @ilianagenkova for long term (ie, operational) plans for processing observations using UFS-DA software.

Is the above sufficient for the time being?

You make a valid point, @WalterKolczynski-NOAA , regarding the relationship between ObsProc and GFS. g-w has long exercised pieces of ObsProc via prep.sh. Now we are adding more observation processing stuff which could / should reside in ObsProc. That said, the ObsProc repo does not maintain a workflow. Therefore, we use g-w to run pieces of ObsProc. What's the best long term solution?

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

I have several questions on this PR:

  • see inline specific comments
  • how does the ioda processing for atmosphere observations in general fit in the scope of the global-workflow? without knowing that the scope or expanse is, it is hard to provide robust review that may also overlap with similar activities that fall under "prep ioda observations for JEDI applications"

jobs/JGLOBAL_PREP_IODA_OBS Outdated Show resolved Hide resolved
parm/config/gfs/config.prepiodaobs Outdated Show resolved Hide resolved
parm/config/gfs/config.prepiodaobs Outdated Show resolved Hide resolved
parm/config/gfs/config.resources Outdated Show resolved Hide resolved
parm/config/gfs/config.resources Outdated Show resolved Hide resolved
@RussTreadon-NOAA
Copy link
Contributor Author

Thank you @aerorahul for your feedback. @CoryMartin-NOAA and @emilyhcliu will need to reply to some of your questions. I took their development and added hooks to exercise it from g-w. You ask excellent design questions which need to go back to source developers.

@CoryMartin-NOAA
Copy link
Contributor

@aerorahul as previously discussed, much of what is here is intended as a stopgap for prototype development and not intended for the final operational implementation. If you would prefer that we take all observation processing out of the workflow and just have the workflow handle ln/cp, we can do so.

@aerorahul
Copy link
Contributor

@aerorahul as previously discussed, much of what is here is intended as a stopgap for prototype development and not intended for the final operational implementation. If you would prefer that we take all observation processing out of the workflow and just have the workflow handle ln/cp, we can do so.

That is only partially true @CoryMartin-NOAA
In the case of marine observations, pre-processing is desired to be inline with the workflow.
Even in the case of atmosphere observations, I was told there will be some aspect of preprocessing of observations that will be relegated to the GFS application, so while this particular implementation of preprocessing is a stopgap for prototype development, the idea is not.
Implementations can change under the hood.

@RussTreadon-NOAA
Copy link
Contributor Author

Rerun 20210814 18 gdas prepatmiodaobs on Hera using RussTreadon-NOAA:feature/iodaobs at cd6972d. Job successfully ran to completion.

@RussTreadon-NOAA
Copy link
Contributor Author

Note: Once GDASApp PR #601 is merged into GDASAPP develop and the updated develop is pulled into GDASApp feature/stable-nightly, we need to update the GDASApp hash in RussTreadon-NOAA:feature/iodaobs Externals.cfg and sorc/checkout.sh.

@RussTreadon-NOAA
Copy link
Contributor Author

Test GDASApp branch feature/bufr2iodapy along with g-w branch RussTreadon-NOAA:feature/iodaobs on Hera and Orion for 20210814 18 gdas. Job prepatmiodaobs ran to completion on both machines. IODA dump files were created for GOES-16 and GOES-17 ABI from the bufr satwnd dump file.

The last update I am aware of for this PR is to update Externals.cfg and sorc/checkout.sh with an updated GDASApp hash once GDASApp PR #601 is merged into develop and feature/stable-nightly is updated.

Does this PR require any additional revisions besides the updated GDASApp hash?

@RussTreadon-NOAA
Copy link
Contributor Author

GDASApp PR #601 was merged into GDASApp develop at 0073a5f. This update was pulled into GDASApp branch feature/stable-nightly at ac8fdb1.

g-w points at GDASApp feature/stable-nightly. Update the GDASApp hash to ac8fdb1 in RussTreadon-NOAA:feature/iodaobs Externals.cfg and sorc/checkout.sh.

Done at 339c6a0.

@RussTreadon-NOAA
Copy link
Contributor Author

Hera and Orion tests
Install RussTreadon-NOAA:feature/iodaobs at 339c6a0 on both machines. Confirm that GDASApp checkout is at the correct hash. Run 20210814 18 gdas prepatmiodaobs. Job successfully ran to completion on both machines.

Work for this PR is done apart from possible requests from reviewers. Re-reviews requested.

@WalterKolczynski-NOAA , do I need to trigger any of the Orion or Hera CI tests using labels?

@WalterKolczynski-NOAA
Copy link
Contributor

@WalterKolczynski-NOAA , do I need to trigger any of the Orion or Hera CI tests using labels?

This is an action we reserve for the g-w code managers. We'll take care of that if/when necessary.

@RussTreadon-NOAA
Copy link
Contributor Author

@WalterKolczynski-NOAA , do I need to trigger any of the Orion or Hera CI tests using labels?

This is an action we reserve for the g-w code managers. We'll take care of that if/when necessary.

Got it. Thank you @WalterKolczynski-NOAA for the clarification.

Is there anything you or @aerorahul need me to do for this PR? I'm done with the changes I intend to make.

Copy link
Contributor

@WalterKolczynski-NOAA WalterKolczynski-NOAA left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Until CI exercises the UFS-DA jobs, I don't think there is much point in CI testing it.

@aerorahul
Copy link
Contributor

The changes look good with the exception that all scripts should be part of the global-workflow if they are executed in the global applications. This has been established for a while now.
This PR adds scripts that are not part of the global-workflow and therefore are hard to manage and maintain a conformity across the project.
We can discuss this offline, but imagine if all the various components started writing and managing their own configuration, scripting, etc, we would be back to where we were.

@WalterKolczynski-NOAA
Copy link
Contributor

WalterKolczynski-NOAA commented Sep 7, 2023

Actually, we do have a test for these now in CI, don't we?

@WalterKolczynski-NOAA WalterKolczynski-NOAA added CI-Hera-Ready **CM use only** PR is ready for CI testing on Hera CI-Orion-Ready **CM use only** PR is ready for CI testing on Orion and removed CI-Hera-Ready **CM use only** PR is ready for CI testing on Hera CI-Orion-Ready **CM use only** PR is ready for CI testing on Orion labels Sep 7, 2023
@RussTreadon-NOAA
Copy link
Contributor Author

The changes look good with the exception that all scripts should be part of the global-workflow if they are executed in the global applications. This has been established for a while now. This PR adds scripts that are not part of the global-workflow and therefore are hard to manage and maintain a conformity across the project. We can discuss this offline, but imagine if all the various components started writing and managing their own configuration, scripting, etc, we would be back to where we were.

Please advise as to the proper action to take to move this PR forward. Tagging @CoryMartin-NOAA, @emilyhcliu , and @guillaumevernieres for awareness.

@WalterKolczynski-NOAA WalterKolczynski-NOAA merged commit 09530b4 into NOAA-EMC:develop Sep 7, 2023
3 checks passed
@aerorahul
Copy link
Contributor

@RussTreadon-NOAA
I didn't mean to hold this PR. I was simply stating what has been a precedent for a while. An issue has been opened now specifically for UFS-DA #1839 and #1840. Discussion can be moved/continued there.

@RussTreadon-NOAA
Copy link
Contributor Author

Thank you @aerorahul for approving this PR. Thank you @WalterKolczynski-NOAA for merging this PR into develop.

@RussTreadon-NOAA RussTreadon-NOAA deleted the feature/iodaobs branch September 7, 2023 19:46
@ilianagenkova
Copy link
Contributor

ilianagenkova commented Sep 12, 2023 via email

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.

Add job to prepare ioda format dumps for UFS-DA
5 participants