NOAA-EMC / GDASApp

Global Data Assimilation System Application
GNU Lesser General Public License v2.1
15 stars 31 forks source link

Bring jcb into GDASapp and use version of jcb not using submodules #1146

Closed danholdaway closed 3 months ago

danholdaway commented 4 months ago

Bring jcb into the GDASapp repo and use the versions of jcb that do not use submodules:

There is a GW branch of the same name and I will make a PR there once this is merged. https://github.com/danholdaway/global-workflow/tree/feature/move_jcb

RussTreadon-NOAA commented 3 months ago

@danholdaway , the changes in GDASApp branch feature/move_jcb have been tested in g-w PR #2665. Everything looks good. g-w PR #2655 has been approved by Cory, Rahul, and me. Another good.

However, I now see a problem.

g-w PR #2655 merges your forked g-w branch danholdaway:feature/move_jcb into g-w develop. The forked g-w feature/move_jcb currently points at gdas.cd @ 0cda73b. This is problematic. This hash is GDASApp branch feature/move_jcb.

I think we want g-w develop to point at GDASApp develop, right? If so, we need to merge this PR into develop and then update the gdas.cd hash in danholdaway:feature/move_jcb accordingly.

danholdaway commented 3 months ago

That is correct @RussTreadon-NOAA. But as soon as we merge GDASapp everything will be broken while we wait for the GW PR to be merged. Alternatively we can merge G-W and then GDASapp and then make another g-w PR changing the hash to develop. That’s the only way that everything continues to work. What do you think? Is that acceptable?

RussTreadon-NOAA commented 3 months ago

@danholdaway , OK. I see your logic. I never thought of having g-w point at a GDASApp branch instead of develop. This works. g-w gdas.cd points at a GDASApp hash. g-w doesn't care if that hash is GDASApp develop, a branch, or a tag.

Your approach allows us to bypass the sequential method I've been using: GDASApp first and then g-w.

g-w PR #2665 can move ahead as is. After this GDASApp PR is merged into GDASApp develop we can decide if we want to open a new g-w PR to point at the updated GDASApp develop or let one of our other GDASApp-based g-w PRs (e.g., #2654, #2641) do this.

danholdaway commented 3 months ago

@RussTreadon-NOAA perhaps in this instance we merge down from g-w to gdas to jcb. Just because it takes longest to get something into g-w wheres we can more quickly merge the others. And in terms of the code pointed to in the hashes it would be equivalent

CoryMartin-NOAA commented 3 months ago

if it's an unchanged squash commit, is the hash identical in the branch and after merged to develop?

RussTreadon-NOAA commented 3 months ago

Works for me. Just want to get as many of your changes into develop as soon as possible.

danholdaway commented 3 months ago

if it's an unchanged squash commit, is the hash identical in the branch and after merged to develop?

No because you have to squash all the commits with a new hash. But the code that gets run would be identical I think.

danholdaway commented 3 months ago

Lets say we merge g-w, then gdas then jcb. Then immediately merged upward jcb, gdas, g-w so that the hashes were the hashes develop points to for each repo...I think the latter would be a sequence of empty PRs. In the g-w case they might even reject the PR, as Rahul did when I tried to bump the hash of GSI utils despite no changes. I might actually try that to see if that theory is correct after these are merged. For non-breaking changes it wouldn't matter which way you merged.

RussTreadon-NOAA commented 3 months ago

g-w PR #2665 was merged into g-w develop at 5af325a (6/13/2024). g-w PR #2665 points g-w gdas.cd at GDASApp branch feature/move_jcb at 368c9c5.

We should merge this PR into GDASApp develop and update the g-w gdas.cd hash to point at the updated GDASApp develop.

emcbot commented 3 months ago

Automated GDASApp Testing Results: Machine: hera

Start: Tue Jun 18 14:42:23 UTC 2024 on hfe03
---------------------------------------------------
Build:                                 *SUCCESS*
Build: Completed at Tue Jun 18 15:30:15 UTC 2024
---------------------------------------------------
Tests:                                 *SUCCESS*
Tests: Completed at Tue Jun 18 15:32:13 UTC 2024
Tests: 100% tests passed, 0 tests failed out of 24
emcbot commented 3 months ago

Automated Global-Workflow GDASApp Testing Results: Machine: hera

Start: Tue Jun 18 15:03:23 UTC 2024 on hfe03
---------------------------------------------------
Build:                                 *SUCCESS*
Build: Completed at Tue Jun 18 15:55:03 UTC 2024
---------------------------------------------------
Tests:                                 *SUCCESS*
Tests: Completed at Tue Jun 18 16:30:44 UTC 2024
Tests: 100% tests passed, 0 tests failed out of 48
RussTreadon-NOAA commented 3 months ago

Orion test Install feature/move_jcb on Orion in g-w develop. Run test_gdasapp. 48 out of 48 tests pass

Test project /work2/noaa/da/rtreadon/git/global-workflow/test/sorc/gdas.cd/build
      Start 1489: test_gdasapp_util_coding_norms
 1/48 Test #1489: test_gdasapp_util_coding_norms ........................   Passed    5.24 sec
      Start 1490: test_gdasapp_util_ioda_example
 2/48 Test #1490: test_gdasapp_util_ioda_example ........................   Passed    4.55 sec

...

      Start 1869: test_gdasapp_atm_jjob_ens_final
47/48 Test #1869: test_gdasapp_atm_jjob_ens_final .......................   Passed   42.23 sec
      Start 1870: test_gdasapp_aero_gen_3dvar_yaml
48/48 Test #1870: test_gdasapp_aero_gen_3dvar_yaml ......................   Passed    0.77 sec

100% tests passed, 0 tests failed out of 48

Label Time Summary:
gdas-utils    =  17.38 sec*proc (11 tests)
script        =  17.38 sec*proc (11 tests)

Total Test time (real) = 1675.41 sec
RussTreadon-NOAA commented 3 months ago

@guillaumevernieres , given successful CI on Hera (automated tests) and Orion, I'd like to merge this PR into develop. Any objections?

guillaumevernieres commented 3 months ago

@guillaumevernieres , given successful CI on Hera (automated tests) and Orion, I'd like to merge this PR into develop. Any objections?

sounds good @RussTreadon-NOAA

RussTreadon-NOAA commented 3 months ago

This PR has two companion PRs

Both have already been approved by @CoryMartin-NOAA. Given this, we should merge these jcb PRs as part of merging this PR. Note that feature/move_jcb already points at the appropriate jcb hashes so we don't need to update jcb hashes used by this PR.