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 policies toward coupling and absorb policies into GDAS/atmosphere #7

Merged
merged 18 commits into from
Jun 20, 2024

Conversation

danholdaway
Copy link
Contributor

@danholdaway danholdaway commented May 24, 2024

In this PR we add more testing and policies ahead of adding more non-atm components to the client (gdas). The new policies are:

  • Client repos have to gather their YAMLs into directories: model, observations, observation_chronicle and algorithm. No directories are allowed outside of those names.
  • Client YAML files in apps/<client>/model/<component> should begin with component. I.e. instead of geometry.yaml they should be atmosphere_geoemtry.yaml. This makes sure that there are no duplicate YAML files between e.g. atmosphere and marine, which could cause problems down the road in a coupling world.
  • Templated client YAML files in apps/<client>/model/<component> should use template keys that are unique to that component, enforced by the keys having to take the form {{<component>_key}}.
  • Model YAMLs should not use any YAML anchors. This could cause problems when YAMLs get combined if anchors are not unique. Since JCB avoid a lot of duplicate YAML writing, anchors aren't really needed anyway for the 'model' YAMLs.
  • Obs YAML files are allowed to use anchors but they have to be unique to that YAML file and the anchor signature has to begin with &{observation_from_jcb} to assert uniqueness.

These policies are mostly designed to alleviate potential problems associated with having several model components in the same config file, for example when coupling. The policies are set now, despite not being needed in the immediate because it's much easier to do so now. Policies can always be revisited later if needed.

Companion PRs in jcb-gdas and jib-algorithms:

@danholdaway danholdaway changed the title Prepare for adding more model components Add policies toward coupling and absorb policies into GDAS/atmosphere May 29, 2024
@danholdaway danholdaway marked this pull request as ready for review May 29, 2024 14:07
Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

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

Approve.

@RussTreadon-NOAA
Copy link
Contributor

g-w PR #2665 removes jcb from g-w. Given this, it seems we can close this PR. Is this correct @danholdaway?

@danholdaway
Copy link
Contributor Author

@RussTreadon-NOAA after jcb moves to GDASapp we can take the change in this PR but into GDASapp.

* feature/no_sub_mods:
  action debugging
  action debugging
  pyyaml
  coding norms
  update README
  refresh testing
  use ref
  update ignore
  remove submodules
RussTreadon-NOAA added a commit to NOAA-EMC/jcb-gdas that referenced this pull request Jun 20, 2024
- Rename the atmosphere YAML files to be `atmosphere_<class>.yaml.j2`
- Change all the keys associated with the atmosphere to be
`{{atmosphere_<key>}}`
- Add the marine YAML files.

---------

Co-authored-by: danholdaway <[email protected]>
Co-authored-by: RussTreadon-NOAA <[email protected]>
@RussTreadon-NOAA
Copy link
Contributor

@CoryMartin-NOAA , I am rerunning the failed JCB tests for various python versions because the first round of tests failed after hitting what appears to be a wall clock limit. The current round of tests appears similarly stuck. I'll let 'em run but I suspect I'll get multiple fails again. If this happens, any suggestions as to how to work around this?

@CoryMartin-NOAA
Copy link
Contributor

@RussTreadon-NOAA do you have a log file or something I can look at? I would think anything related to JCB should finish very quickly, so something strange must be going on.

@CoryMartin-NOAA
Copy link
Contributor

@RussTreadon-NOAA or are you talking about the GitHub actions?

@RussTreadon-NOAA
Copy link
Contributor

@RussTreadon-NOAA or are you talking about the GitHub actions?

Yes, @CoryMartin-NOAA, I am referring to the github actions. They appear to be hung. This is what happened on the previous run.

@CoryMartin-NOAA
Copy link
Contributor

Screenshot 2024-06-20 at 2 38 33 PM Do you see this? Or do only I have the "destructive power"? Looks like I can bypass it.

@RussTreadon-NOAA
Copy link
Contributor

@CoryMartin-NOAA: Yes, I see the same Merge without waiting checkbox. I am reluctant to bypass protections since I am not sure if the failed tests indicated a problem with the PR (not acceptable) or something strange with actions (acceptable).

@CoryMartin-NOAA
Copy link
Contributor

It seems like it is a GitHub actions issue. Can we maybe try running JCB ctests on HPC using this branch? (I've never done this before)

@CoryMartin-NOAA
Copy link
Contributor

I manually checked out this branch on Hera, ran the commands in the CI, and all 23 tests pass. I'm going to merge and if this comes up again, we will identify and fix why the GitHub Actions are timing out.

@CoryMartin-NOAA CoryMartin-NOAA merged commit 1639932 into develop Jun 20, 2024
1 of 6 checks passed
@CoryMartin-NOAA CoryMartin-NOAA deleted the feature/rename_atm branch June 20, 2024 20:50
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.

4 participants