ESMCI / cime

Common Infrastructure for Modeling the Earth
http://esmci.github.io/cime
Other
161 stars 206 forks source link

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

Closed lizziel closed 6 months ago

lizziel commented 6 months ago

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 commented 6 months ago

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

lizziel commented 6 months ago

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

jedwards4b commented 6 months ago

@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 commented 6 months ago

@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 https://github.com/ESMCI/cime/pull/4604/commits/091c4971025fc66b09d947322cedb8310e4aac08 and https://github.com/ESMCI/cime/pull/4604/commits/a508c9d26c425fcfeba7ea574b2e483d3d903b92 into the PR.

lizziel commented 6 months ago

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

lizziel commented 6 months ago

@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 commented 6 months ago

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

lizziel commented 6 months ago

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

jedwards4b commented 6 months ago

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 commented 6 months ago

All set, but tests are still failing.

jedwards4b commented 6 months ago

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

jedwards4b commented 6 months ago

@jasonb5 containers are still failing.

lizziel commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

github container testing appears broken again.

lizziel commented 6 months ago

Thank you!