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

Updates from EMC master #14

Conversation

christopherwharrop-noaa
Copy link
Collaborator

@christopherwharrop-noaa christopherwharrop-noaa commented Apr 18, 2022

This PR merges updates from the EMC master branch to keep GSL's feature/rrfs-cmake-refactor synchronized with recent EMC updates.

It also fixes a parameter string array length bug in gsi_rfv3io_mod that was preventing successful builds with Gnu on Cheyenne.

HaixiaLiu-NOAA and others added 15 commits April 5, 2022 03:53
…r_n21, ompsnp_n21, ompstc8_n21, gome_metop-c, abi_g18, ahi_himawari9, viirs-m_j2
…ce in situ observations, modify global_convinfo.txt to loose O-B QC test for in situ observations, and modify exglobal_atmos_analysis.sh for smaller dmesh for surface sensetive radiances for GSI/NSST
GitHub Issue NOAA-EMC#344. Changes to monitor new observations from N21, G18, Himawari9
Address GFS adaptation for NCO requirement of "pruning"
Add BUILD_UTIL_ALL option for convenience
@christopherwharrop-noaa
Copy link
Collaborator Author

christopherwharrop-noaa commented Apr 18, 2022

@hu5970 - I see un-gated pushes to feature/rrfs-cmake-update. Because of the way the cmake refactoring works and the method that Rahul Mahajan is using to keep his work up-to-date with GSI master at EMC, I recommend the following approach for keeping GSI up-to-date:

  1. Merge Rahul's feature/cmake-update branch into GSL feature/cmake-refactor
  2. Add GSL-specific updates to feature/cmake-refactor
  3. Issue a PR to feature/rrfs-cmake-refactor and test to make sure updates are OK
  4. Merge the PR

If you push directly to feature/rrfs-cmake-refactor without incorporating updates from Rahul's branch, you risk divergence that causes conflicts. Rahul keeps his cmake refactor branch up to date with EMC master so that his PR is up to date while under consideration.

Because the cmake refactor is a large change the best way to incorporate changes from EMC master is to let Rahul merge those into his work first. Then, take Rahul's changes into GSL's fork. Merging directly from EMC master will involve resolving a lot of complicated conflicts, so it's best to let Rahul do that work since he knows how to resolve them.

@christopherwharrop-noaa christopherwharrop-noaa changed the title Update to EMC master Updates from EMC master Apr 18, 2022
@hu5970
Copy link

hu5970 commented Apr 19, 2022

@christopherwharrop-noaa The feature/rrfs-cmake-refactor branch is used for holding new development and bug fix for RRFS test for now. It does not need to be synced with master. Please work with Rahul to make the make_refactor branch merged with EMC master. I will commit changes in feature/rrfs-cmake-refactor branch to EMS master. After a those two PR merged with EMC master, we can let the RRFS to use master directly. We will create a new branch if we need further development from master. So, all our local branch will be terminated soon.

@christopherwharrop-noaa
Copy link
Collaborator Author

christopherwharrop-noaa commented Apr 19, 2022

@hu5970 - I'm not sure I'm following you. It would be wise for you to keep feature/rrfs-cmake-refactor up-to-date with GSI master because otherwise you will be making bugfixes and development changes to out-dated code and that has the potential to introduce complicated merge conflicts later. Changes need to be made to the latest code to be sure they are correctly applied and also to ensure that scientific tests and results are meaningful. Right now, Rahul's feature/cmake-update branch is the authoritative copy of EMC's GSI containing the cmake refactor. Rahul is keeping it up-to-date and synchronized with all updates to EMC's master as pull requests at EMC are merged. His PR is expected to be merged in about three weeks. Therefore the simplest, and most straightforward, method for keeping GSL's feature/rrfs-cmake-refactor updated with EMC's master is to periodically merge Rahul's feature/cmake-update into it. This ensures that GSL's local changes are made on top of the latest code, which is important for maintaining both code correctness and scientific integrity of the changes you are making. You are correct that things will become much simpler once Rahul's PR is merged onto EMC's master. In the meantime, we can keep feature/rrfs-cmake-refactor synchronized with EMC master such that local changes are made in a consistent and correct fashion.

@christopherwharrop-noaa
Copy link
Collaborator Author

christopherwharrop-noaa commented Apr 19, 2022

FYI, there are currently 3 PRs ahead of Rahul's in the pipeline at EMC. They are:

1) PR NOAA-EMC#354 - GPS RO Updates
2) PR NOAA-EMC#350 - Update the use of MW radiances for GFS v16.3 implementation
3) PR NOAA-EMC#355 - The inclusion of of the mixed surface type in situ observations and looser QC for in situ observations

@hu5970
Copy link

hu5970 commented Apr 19, 2022

@christopherwharrop-noaa All those master branch updates will not impact the GSI results in RRFS. We don't need to spend time to follow master branch in next several weeks util the PR (from Rahul and me) merged into master branch.
If there are any local change in feature/rrfs-cmake-refactor need to be carried, we will manually add them on top of the master branch.

@christopherwharrop-noaa
Copy link
Collaborator Author

@hu5970 - I am not understanding the problem here. Developers should not be making local changes to old code. They should be making them to the latest code. This is basic distributed software engineering development best practice and common sense. This PR updates the code to the latest so that changes, which I see are being pushed directly rather than via PR, can be made knowing they are being tested in the context of the latest code. This PR also reinstates a bugfix that Rahul made on Feb 24 which fixes the Gnu build. Having to reinstate previous fixes is inefficient and is often a consequence of letting code diverge and making changes to old code instead of keeping up-to-date. Please clearly explain what the problem is with merging this. Is there a problem with the content (updates from EMC master) of this PR? If so, please point them out.

@christinaholtNOAA
Copy link
Collaborator

@hu5970 We really cannot work with this scenario in ways other than what @christopherwharrop-noaa is proposing here. We are in a pretty delicate balance here where many different branches need to be simultaneously treated with very strict code management best practice. Chris has been doing an excellent job managing that for us!

During this transition phase where we need Rahul's changes to go into master, and we are holding onto his changes here in our Fork, we really need to do things by the book. Otherwise we will have a huge mess to sort out.

@guoqing-noaa
Copy link
Collaborator

I agree that it is good to keep sync'ed with the EMC master. However, in order to do this, we will first need to make sure a few regional regression tests are in place in the EMC master so that PR's there will not break regional applications. In the past few years, it happened a lot of times that updates at the EMC master broke regional applications.

@christopherwharrop-noaa
Copy link
Collaborator Author

Keep in mind that you do not have to adopt the latest GSI hash for the App. So lots of testing can occur without impacting that. I sympathize with incoming PRs breaking regional applications. However, for all intents and purposes, you don't really have a choice in taking the updates from master because that is the authoritative codebase and it is the one all your development must work with. You should be keeping up to date with it, and testing and applying bug fixes on top of that code. You are already opening PRs to EMC master to add bug fixes, and that is good. Meanwhile, master advances, so you still need to bring in those updates, test, and fix as needed. This is all a normal part of distributed development.

@guoqing-noaa
Copy link
Collaborator

This may be a good opportunity to make the unification go a further step. Previously the global and regional GSI developments are detached. Most EMC efforts were on the global part and the regional part was overlooked. That's why lots of times updates there broke the regional part and it was also not rare that some new updates even overwrote already-merged/committed bug fixes from the regional developers. If there are some basic regional regression tests, lots of duplicate work can be avoided. So it will be good to have both global and regional regression tests at the EMC master so as to unify the development efforts.

@christopherwharrop-noaa
Copy link
Collaborator Author

@guoqing-noaa - If you have any suggestions for regression tests to add that exercise the regional applications, I think the GSI code manager would be interested in hearing those. It's in everyone's best interest to keep the code working for both global and regional applications. Perhaps opening an issue in the GSI EMC repository stating the need for regional regression tests, and outlining a few suggestions might be helpful?

@hu5970
Copy link

hu5970 commented Apr 20, 2022

Hi All, thanks for good discussion on those code management issues. For now, Let's focus on one branch:

The feature/rrfs-cmake-refactor branch is used for RRFS test. Only code changes (bug fix, new development) will be used for RRFS should be commit to this branch. It does not need to sync with the master as we do not want to use master code for RRFS. I believe sync with master in near future is easy even it is not follow the master now as we know what has been done in this branch.

If future discussion is needed, I would suggest we have meeting to talk about ideas/methods to best manage this branch for RRFS implementations.

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.

9 participants