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

Fix bug in path to CAM script cam.case_setup.py #4604

Merged

Conversation

lizziel
Copy link
Contributor

@lizziel lizziel commented Mar 21, 2024

Just prior to the cime6.0.217 tag there was a change in the original code to call cam.case_setup.py
during case setup. The change added a conditional for file path existence, but the file path added was
incorrect. This causes the script to never be called and GEOS-Chem compsets will subsequently all fail.

Tagging @cacraigucar since this bug makes me wonder if GEOS-Chem tests are running, or if I am missing something else that changed that would make the existing code work.

Test suite: N/A
Test baseline: N/A
Test namelist changes: N/A
Test status: [bit for bit, roundoff, climate changing]

Fixes [CIME Github issue #] none

User interface changes?: N

Update gh-pages html (Y/N)?: N

@jasonb5
Copy link
Collaborator

jasonb5 commented Mar 22, 2024

@lizziel Please sync with master, this will clear up the failed check.

@lizziel lizziel force-pushed the bugfix/path_to_cam_case_setup_script branch from 7cb54ca to fde0e2f Compare March 22, 2024 19:42
@lizziel
Copy link
Contributor Author

lizziel commented Mar 22, 2024

All set. I had to add CIME/non_py/cprnc during my rebase but it looks like the rebase worked out fine.

@jedwards4b
Copy link
Contributor

@lizziel this branch is messed up with respect to master, there are a lot of changes I'm sure that you did not intend. Can you try again?

@jasonb5
Copy link
Collaborator

jasonb5 commented Mar 28, 2024

@lizziel @jedwards4b This was my mistake, I asked to sync with master and missed it being based on cime6.0.217_httpsbranch. We'll need to cherry-pick 091c497 and a508c9d into the PR.

@lizziel
Copy link
Contributor Author

lizziel commented Mar 28, 2024

Let me know if you want me to rebase on a different commit.

@lizziel
Copy link
Contributor Author

lizziel commented Apr 3, 2024

@jedwards4b, do you have an idea of when you can apply this fix? I can rebase it onto whatever target branch you plan to merge it into. Without this fix GEOS-Chem will not work in CAM.

@jedwards4b
Copy link
Contributor

I don't want to see a commit that changes 25 files - fix that and we can merge.

@lizziel lizziel changed the base branch from cime6.0.217_httpsbranch to master April 3, 2024 15:56
@lizziel lizziel force-pushed the bugfix/path_to_cam_case_setup_script branch from fde0e2f to 8258b5f Compare April 3, 2024 16:02
@lizziel
Copy link
Contributor Author

lizziel commented Apr 3, 2024

I assume you want this targeting master so I cherry-picked to that. It is now back to the simple directory path fix.

@jedwards4b
Copy link
Contributor

Looks better, now we just need to get tests working, black wants the following change:

-                if os.path.exists(os.path.join(camroot, "cime_config/cam.case_setup.py")):
+                if os.path.exists(
+                    os.path.join(camroot, "cime_config/cam.case_setup.py")
+                ):

@lizziel lizziel force-pushed the bugfix/path_to_cam_case_setup_script branch from 8258b5f to d9176d7 Compare April 3, 2024 16:52
@lizziel
Copy link
Contributor Author

lizziel commented Apr 3, 2024

All set, but tests are still failing.

@jedwards4b
Copy link
Contributor

You introduced an indentation error, 428 is indented too far. You should install pre-commit and use it, would save a lot of time.

Just prior to the cime6.0.217 tag there was a change in the original code
to call cam.case_setup.py during case setup. The change added a
conditional for file path existence, but the file path added was incorrect.
This causes the script to never be called and GEOS-Chem compsets will
subsequently all fail.

Signed-off-by: Lizzie Lundgren <elundgren@seas.harvard.edu>
@lizziel lizziel force-pushed the bugfix/path_to_cam_case_setup_script branch from d9176d7 to 5dbce34 Compare April 3, 2024 19:26
@jedwards4b
Copy link
Contributor

@jasonb5 containers are still failing.

@lizziel
Copy link
Contributor Author

lizziel commented Apr 3, 2024

Sorry, I am not familiar with these tests. Could you interpret the error message for me?

  /usr/bin/docker --config /home/runner/work/_temp/.docker_21a9cd05-c822-442e-9394-3db07e1f[16](https://github.com/ESMCI/cime/actions/runs/8544409469/job/23410369150?pr=4604#step:2:19)c6 pull ghcr.io/esmci/cime:latest
  latest: Pulling from esmci/cime
  manifest unknown
  Warning: Docker pull failed with exit code 1, back off 1.139 seconds before retry.

@jedwards4b
Copy link
Contributor

@lizziel this is a problem we've been having recently with the cime testing, I don't think it's your PR.
I'm going to make an executive decision and just merge it - this has gone on long enough...

@jedwards4b
Copy link
Contributor

github container testing appears broken again.

@jedwards4b jedwards4b merged commit adebed5 into ESMCI:master Apr 3, 2024
2 of 7 checks passed
@lizziel
Copy link
Contributor Author

lizziel commented Apr 3, 2024

Thank you!

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.

3 participants