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

Update fates_next_api to ctsm1.0.dev093 #1050

Conversation

glemieux
Copy link
Collaborator

@glemieux glemieux commented Jun 19, 2020

Description of changes

This PR reconciles differences between release-clm5.0.30 and ctsm1.0.dev093 (https://github.com/glemieux/ctsm/tree/release-clm5.0.30-ctsm1.0.dev093) and then pre-merges these changes into fates_next_api.

Specific notes

This PR addresses #1008 in part. Discussion of release-clm5.0.30 and ctsm1.0.dev093 merge deconflict is in found glemieux#1.

Contributors other than yourself, if any: @ekluzek

CTSM Issues Fixed (include github issue #):

Are answers expected to change (and if so in what way)? yes

Any User Interface Changes (namelist or namelist defaults changes)?

Testing performed, if any:
Both aux_clm and fates regression test suites were run using run_sys_tests:

/glade/scratch/glemieux/ctsm-tests/tests_ctsmdev-release-merge-fullregression
/glade/scratch/glemieux/ctsm-tests/tests_testfates_next_api-merge

billsacks and others added 30 commits January 27, 2020 15:52
Point to a slightly modified version that has updates for some izumi
machine changes
Rename variables to avoid confusion; fix QSNOEVAP diagnostic

Renamed variables as discussed in ESCOMP#118 throughout the code.

Also made a couple of minor changes to fix a couple of potential
problems with these variables as described in the branch commit logs.

Tested for bfb before changing the history field variables
themselves. These changes are all bfb (with the exception of QSNOEVAP)
for a 1 month global 2deg simulation, but may not be bfb under all
conditions. QSNOEVAP is not bfb because I've added in qflx_ev_snow for
lakes.

Also, update cime slightly with a fix for izumi machine updates

Resolves ESCOMP#118
…x1_Ln3.hcru_hcru.I2000Clm50BgcCruGs.cheyenne_intel.clm-coldStart) pass for cmpes. (Issue ESCOMP#874).
@glemieux glemieux added FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM PR status: work in progress labels Jun 19, 2020
@billsacks
Copy link
Member

@glemieux I'm not sure I totally understand what's going on here, so perhaps I shouldn't wade into this. But seeing the huge number of commits here makes me wonder if this was done right. I'm particularly concerned about what will happen when we go to move this branch back to master.

My understanding is that fates_next_api is currently based off of release-clm5.0, but you want it based off of master. My further understanding (maybe wrong) is that the branch in this PR accomplishes this via a merge together with a lot of conflict resolution.

However, I see:

$ git rev-list --count escomp/release-clm5.0..escomp/fates_next_api
79

i.e., there are 79 commits on fates_next_api beyond the release-clm5.0 branch, yet:

$ git rev-list --count escomp/master..glemieux/fates_next_api-release-clm5.0.30-ctsm1.0.dev093
855

i.e., there are 855 commits on the branch in this PR beyond master.

Even if the end result is correct, I'm concerned about what this might do to the history of master once this branch is brought back to master.

I would typically accomplish something like this with a rebase:

git checkout -b fates_next_api_master escomp/fates_next_api
git rebase --onto escomp/master escomp/release-clm5.0 fates_next_api_master

I'd be happy to discuss this further if it would be helpful. Also, feel free to let me know if you think I'm misunderstanding something (which is quite possible).

@glemieux
Copy link
Collaborator Author

@billsacks I don't think you're misunderstanding. This is my first try at attempting something like this so I'd be willing to scrap this for another method. Part of the reason I created the PR was to get it in front of y'all to help critique it. I've never rebased before so I'm willing to give that a shot.

@billsacks
Copy link
Member

Okay, sounds good, thanks. I did just try doing the rebase myself, and found that (perhaps unsurprisingly) there were a number of conflicts. I didn't go far: I just spent a few minutes exploring this. A few things to note about rebases (since you say you haven't done them before):

  • Git will apply commits one-by-one, from oldest to newest; it will pause whenever it hits a conflict in trying to apply the diffs from a given commit. So, in contrast to a merge, where you resolve conflicts on the whole kit and caboodle all at once, with a rebase you resolve conflicts one commit at a time (for better or for worse).

  • It will drop any merge commits (and their associated log messages) from the history. If this is an issue (e.g., because there is important historical information in the merge commit messages), I've come up with workflows to maintain these merge commits, but they are more tedious.

Again, let me know if you'd like help with this - for example, we could find some time to do it together over zoom.

@glemieux
Copy link
Collaborator Author

Thanks for the quick overview. I think a zoom call would be a good idea. I sent you an email to coordinate a time to meet up.

@glemieux glemieux closed this Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FATES API update Changes to the FATES version that also REQUIRE an API change in CTSM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants