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 CIME submodule #6565

Merged
merged 5 commits into from
Sep 5, 2024
Merged

Update CIME submodule #6565

merged 5 commits into from
Sep 5, 2024

Conversation

jgfouca
Copy link
Member

@jgfouca jgfouca commented Aug 26, 2024

... to 52787427846eec58038ab59664fd9c2706f23513 (which is jgfouca/e3sm_cime, not latest master, due to a couple things that needed to be reverted off CIME master).

From cime6.0.248

Changes:

  1. Generalize _hists_match for mom6 files
  2. support append in config_pes.xml overrides field
  3. Add case git: Adds a case interface to git.
    It creates a local git repository when case.setup is
    run and updates that repository with each action that triggers an
    update of the CaseStatus file. Optionally a remote repository can be
    attached by setting CASE_GIT_REPOSITORY to the name of the remote
    repository. DISABLED FOR NOW
  4. xmlchange: Add documentation for setting values with commas
  5. Remove mct for cesm
  6. remove load_balancing_tool, broken and unsupported

Fixes

  1. cprnc: need to make sure the directory exists before linking to it
  2. check_lockedfiles call in xmlchange needs quiet=True
  3. fixes detection of pfunit_path by making sure it exists
  4. Fixes handling cprnc output
  5. Fixes creating new environment with specific python version
  6. Fixes query_config tool.
    • Adds longname to --grids
    • Fixes duplicate and empty choices
  7. Fixes check_lockedfiles call in xmlchange
  8. Fix list_e3sm_tests tool

[BFB]

... to af3eab5b8a213e79923551e7f9d9f56edb944c0f

Changes:
1) Generalize _hists_match for mom6 files
2) support append in config_pes.xml overrides field
3) Add case git: Adds a case interface to git.
   It creates a local git repository when case.setup is
   run and updates that repository with each action that triggers an
   update of the CaseStatus file. Optionally a remote repository can be
   attached by setting CASE_GIT_REPOSITORY to the name of the remote
   repository.
4) xmlchange: Add documentation for setting values with commas
5) Remove mct for cesm
6) remove load_balancing_tool, broken and unsupported

Fixes
1) Fix/rest n in tests: move the computation of REST_N into the python
2) cprnc: need to make sure the directory exists before linking to it
3) check_lockedfiles call in xmlchange needs quiet=True
4) fixes detection of pfunit_path by making sure it exists
5) Fixes handling cprnc output
6) Fixes creating new environment with specific python version
7) Fixes `query_config` tool.
   - Adds longname to `--grids`
   - Fixes duplicate and empty choices
8) Fixes check_lockedfiles call in xmlchange
9) Fix list_e3sm_tests tool

[BFB]
@jgfouca jgfouca added BFB PR leaves answers BFB CIME labels Aug 26, 2024
@jgfouca jgfouca self-assigned this Aug 26, 2024
Copy link

github-actions bot commented Aug 26, 2024

PR Preview Action v1.4.7
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6565/
on branch gh-pages at 2024-09-04 16:17 UTC

Copy link
Contributor

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

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

Add case git: Adds a case interface to git.
It creates a local git repository when case.setup is
run and updates that repository with each action that triggers an
update of the CaseStatus file. Optionally a remote repository can be
attached by setting CASE_GIT_REPOSITORY to the name of the remote
repository.

this breaks our containerized testing. Is there a way to disable this breaking change?

@jgfouca
Copy link
Member Author

jgfouca commented Aug 26, 2024

@mahf708 , that feature is off by default.

@jgfouca
Copy link
Member Author

jgfouca commented Aug 26, 2024

Oh, I see the CI fails. I guess it breaks even if off? Nevermind, I can see it's trying to do git operations. I will look at it.

@mahf708
Copy link
Contributor

mahf708 commented Aug 26, 2024

Yes, this feature is relatively invasive if I understand it correctly. It requires one to have a whole functioning git setup (so dealing with auth, etc.) regardless of if one wants to keep this type of provenance or not. All in order to document how case setup was run? I don't understand the advantage. But more importantly, this is an invasive breaking change, which I would put under a big warning (potentially with a few cycles of deprecation/warnings notices). Besides our tests in containers, this also will break case running for people who run without git (e.g., from a tar ball with all modules, all-batteries-included) or people who don't necessary want to set up git with all its procedures (e.g., auth, etc.)

Users who wish to use the CASE_GIT_REPOSITORY feature must have write permissions to the remote repository and it is recommended that the ssh interface to that repository be used to avoid frequent prompts for github tokens.

@jgfouca
Copy link
Member Author

jgfouca commented Aug 26, 2024

Yeah, I thought this was something the user had to opt-in for.

Copy link
Contributor

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

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

All good, thanks. Note that we can get around this if we need to (by enforcing users/containers have proper setup, etc.). Let me know if you'd like that done for the containers and I can implement it.

@mahf708
Copy link
Contributor

mahf708 commented Aug 26, 2024

Also, I may have overstated the implications of this change. I thought the feature created a branch on an existing repo and pushed it. But I think I have misunderstood. It seems it should only create a brand new local repo for provenance. My bad.

@mahf708
Copy link
Contributor

mahf708 commented Aug 28, 2024

@jgfouca @rljacob looks like this is an actual fail we are seeing?

ERS.ne4pg2_oQU480.WCYCL2010NS.machine_gnu.allactive-wcprod_1850

I tried replicating locally, but I ran into other errors:

            if append[key]:
        KeyError: 'MAX_MPITASKS_PER_NODE'

point to an edit in the diff: ESMCI/cime@f903115...eacb1b7#diff-0b3cc73813f3cb516e9c0674d6968742511c7fb7366be4b71fbec67061fb81f4R1237

@rljacob
Copy link
Member

rljacob commented Aug 28, 2024

But ERS_P8.ne4pg2_oQU480.WCYCL2010NS passes and the only difference is the testmod to add output.

@mahf708
Copy link
Contributor

mahf708 commented Aug 28, 2024

But ERS_P8.ne4pg2_oQU480.WCYCL2010NS passes and the only difference is the testmod to add output.

Yeah, the fail is in the compare stage:

FAIL ERS_P8.ne4pg2_oQU480.WCYCL2010NS.singularity2_gnu.allactive-wcprod_1850 (phase COMPARE_base_rest)
    Case dir: /github/home/projects/e3sm/scratch/ERS_P8.ne4pg2_oQU480.WCYCL2010NS.singularity2_gnu.allactive-wcprod_1850.20240827_150640_vxbxom

We can download all artifacts. Let me examine them

@mahf708
Copy link
Contributor

mahf708 commented Aug 28, 2024

All cprnc files show IDENTICAL results. I am not sure what went wrong there. Note that there are only 6 cprnc files, 5 of which atm. Notably, the eam h0 one is missing.

@rljacob
Copy link
Member

rljacob commented Aug 28, 2024

This is the error message: “Expected to compare 9 hist files, but only compared 6. It’s possible
that the hist_file_extension entry in config_archive.xml is not correct
for some of your components.”

Copy link
Member

@rljacob rljacob left a comment

Choose a reason for hiding this comment

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

Need to fix ERS test. Should compare 9 files but only compares 6.

jgfouca added a commit that referenced this pull request Aug 29, 2024
Update CIME submodule

... to af3eab5b8a213e79923551e7f9d9f56edb944c0f
cime6.1.18

From cime6.0.248

Changes:
1) Generalize _hists_match for mom6 files
2) support append in config_pes.xml overrides field
3) Add case git: Adds a case interface to git.
   It creates a local git repository when case.setup is
   run and updates that repository with each action that triggers an
   update of the CaseStatus file. Optionally a remote repository can be
   attached by setting CASE_GIT_REPOSITORY to the name of the remote
   repository.
4) xmlchange: Add documentation for setting values with commas
5) Remove mct for cesm
6) remove load_balancing_tool, broken and unsupported

Fixes:
1) Fix/rest n in tests: move the computation of REST_N into the python
2) cprnc: need to make sure the directory exists before linking to it
3) check_lockedfiles call in xmlchange needs quiet=True
4) fixes detection of pfunit_path by making sure it exists
5) Fixes handling cprnc output
6) Fixes creating new environment with specific python version
7) Fixes query_config tool.
  * Adds longname to --grids
  * Fixes duplicate and empty choices
8) Fixes check_lockedfiles call in xmlchange
9) Fix list_e3sm_tests tool

[BFB]
@jgfouca
Copy link
Member Author

jgfouca commented Aug 29, 2024

Merged to next.

@ndkeen
Copy link
Contributor

ndkeen commented Aug 30, 2024

Yes we hit that error that @mahf708 reported

            if append[key]:
        KeyError: 'MAX_MPITASKS_PER_NODE'

jgfouca added a commit that referenced this pull request Aug 30, 2024
Update CIME submodule

Merge 2 for this PR.

[BFB]
jgfouca added a commit that referenced this pull request Sep 3, 2024
... to af3eab5b8a213e79923551e7f9d9f56edb944c0f
cime6.1.18

From cime6.0.248

Changes:
1) Generalize _hists_match for mom6 files
2) support append in config_pes.xml overrides field
3) Add case git: Adds a case interface to git.
4) It creates a local git repository when case.setup is
   run and updates that repository with each action that triggers an
   update of the CaseStatus file. Optionally a remote repository can be
   attached by setting CASE_GIT_REPOSITORY to the name of the remote
   repository.
5) xmlchange: Add documentation for setting values with commas
6) Remove mct for cesm
7) remove load_balancing_tool, broken and unsupported

Fixes:
1) Fix/rest n in tests: move the computation of REST_N into the python
2) cprnc: need to make sure the directory exists before linking to it
3) check_lockedfiles call in xmlchange needs quiet=True
4) fixes detection of pfunit_path by making sure it exists
5) Fixes handling cprnc output
6) Fixes creating new environment with specific python version
7) Fixes query_config tool.
  * Adds longname to --grids
  * Fixes duplicate and empty choices
8) Fixes check_lockedfiles call in xmlchange
9) Fix list_e3sm_tests tool

[BFB]

* jgfouca/update_cime_subm_2024_08_26:
  Fix append feature
  Revert ers change
  Disable git interface
  Update CIME submodule
@jgfouca
Copy link
Member Author

jgfouca commented Sep 3, 2024

Re-merged to next.

@rljacob
Copy link
Member

rljacob commented Sep 3, 2024

Is the PR description still right? Are we updating to hash af3eab5b8 ?

@jgfouca
Copy link
Member Author

jgfouca commented Sep 3, 2024

@rljacob , fixed.

@rljacob
Copy link
Member

rljacob commented Sep 4, 2024

But you're not updating to a hash on master are you? Its a hash (52787427846) on a branch off of master to revert the git feature and other things. The PR description needs to say that.

@jgfouca
Copy link
Member Author

jgfouca commented Sep 4, 2024

@rljacob , done.

@rljacob
Copy link
Member

rljacob commented Sep 4, 2024

The description still isn't accurate because the git feature is disabled and the rest_n calculation PR was completely reverted so that had to be removed. I fixed it. Also that branch has to live forever in ESMCI/cime so that this state of master can be recovered.

jgfouca added a commit that referenced this pull request Sep 4, 2024
* jgfouca/update_cime_subm_2024_08_26:
  Another fix for CIME mvk and pgn

Merge 3 for this PR.

[BFB]
@jgfouca jgfouca merged commit fa92e85 into master Sep 5, 2024
13 checks passed
@jgfouca jgfouca deleted the jgfouca/update_cime_subm_2024_08_26 branch September 5, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB CIME
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants