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

Suggested changes to cam.case_setup.py #26

Closed

Conversation

gold2718
Copy link

@gold2718 gold2718 commented Oct 2, 2023

I added several suggestions here.
I think many of them are fixes to an incomplete first pass by you.

The hard bit is likely to be with the multi-instance code. In CESM, we usually allow runtime configuration files to be specified on a per-instance basis. I added this code to the copy part of the script. However, the GEOS-Chem code that reads these files has no idea what is going on. I think you should discuss this with the AMP SEs and see if they want to push through this or to not have GEOS-Chem supported for multi-instance runs (a different but probably smaller chore).

Let me know if you have questions.
--Steve

@gold2718
Copy link
Author

gold2718 commented Oct 3, 2023

BTW, I do not have the ability to test this yet so it probably has bugs. (probability ~100%)

@lizziel lizziel force-pushed the CESM-GC_rebased_on_cam6_0_034 branch from a252cf6 to d43c4e8 Compare October 4, 2023 19:47
@lizziel lizziel force-pushed the CESM-GC_rebased_on_cam6_0_034 branch 2 times, most recently from 1c3ec21 to da76f71 Compare October 17, 2023 21:09
sys.path.insert(0, _CIMEROOT)

#pylint: disable=wrong-import-position
from CIME.case import Case
Copy link

Choose a reason for hiding this comment

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

Is this and the above to CIMEROOT all for getting case for the logger and cam options?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Most of CIME works off of a case object and this is the standard way to bootstrap one of those.

if comp_interface == "nuopc":
ninst = case.get_value("NINST")
elif ninst == 1:
ninst = case.get_value("NINST_CAM")
Copy link

Choose a reason for hiding this comment

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

Could you explain what ninst is and how it is different for nuopc versus non-nuopc?

Copy link
Author

Choose a reason for hiding this comment

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

CESM has the capability to run an ensemble of instances as a single run. This is most often used as part of a data assimilation (DA) cycle. The ensemble runs, say for 6 hours, then writes restart files and stops while DART updates the state. Next, the model restarts with the updated state. This entire process can run several times as a single job submission (longer runs are handled with automatic job resubmission).

When the multiple instance implementation was first created in MCT, one idea was to be able to have some components run a single instance while others ran several (for instance, a single ocean model responding to an ensemble of atmosphere simulations). However, no one ever figured out how to define this scientifically (coupling is hard) so the idea was officially dropped many years ago. The move to NUOPC just formalizes that situation.

The impact on GEOS-CHEM is deciding if it will support different configurations for different instances of CAM running as a single job. CAM will have atm_in_0001, atm_in_0002 etc. but that has an impact inside of CAM where it has to check its status as an instance and look for the correct namelist filename to open. I have no idea if this is a priority for the community that will be using GEOS-CHEM which is why they should decide if the (not insignificant) extra effort is worth it. If they decide they do not need full multi-instance support, I suggest it is either prevented (perhaps in this script) or it is just specified that all instances will use the same set of GEOS-CHEM configuration files.

Copy link

@lizziel lizziel Oct 20, 2023

Choose a reason for hiding this comment

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

Ah, okay. We decided not to enable multi-instance support, at least for now. However, if we can enable it with the constraint that all GEOS-Chem config files are identical then that would be a first step. There may be use cases for multi-instance with varying CAM settings that are external to the GEOS-Chem files. I see now this is what you implemented, correct?

Copy link
Author

Choose a reason for hiding this comment

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

Not quite. I think you need to remove lines 56-59 (and 61).
Perhaps leave a comment saying that those files will be the same on all instances for a multi-instance case.

spaths = source_file.splitext(source_file)
for inst_num in range(ninst):
if ninst > 1:
target_file = f"{spaths[0]}_{inst_num+1:04d}{spaths[1]}"
Copy link

Choose a reason for hiding this comment

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

I don't quite understand this part. Maybe your explanation of nuopc above will explain. If not, could you detail what is happening here that is special for nuopc?

Copy link
Author

Choose a reason for hiding this comment

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

I don't quite understand this part. Maybe your explanation of nuopc above will explain. If not, could you detail what is happening here that is special for nuopc?

I think this suggested change is independent of MCT / NUOPC. It is also only relevant if GEOS-CHEM does decide to go ahead with support for multiple instance runs with independent GEOS-CHEM runtime configurations.
What this was intended to do is add an instance qualifier without disturbing the filename extension. So

species_database.yml ==> species_database_0001.yml

and

geoschem_config.yml ==> geoschem_config_0001.yml

and

HISTORY.rc ==> HISTORY_0001.rc

It is just a suggestion for how to create multiple instance files for a multi-instance run. I also have no idea if it works :)

# end if
# end for
# end for
# end if
Copy link

Choose a reason for hiding this comment

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

Do you mind if I leave out the end comments?

Copy link
Author

Choose a reason for hiding this comment

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

They are part of the CAM Python coding standards so you would probably have to put them back when you get to the CAM SEs.
(note, there are also Fortran coding standards).

Copy link

Choose a reason for hiding this comment

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

Got it.

Copy link

@lizziel lizziel left a comment

Choose a reason for hiding this comment

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

Thanks @gold2718. Sorry it took me so long to look at this. What I have in the cime+cam PRs works but I can put in these improvements. I just have a few clarifying questions to better understand what you put, particularly with regards to nuopc.

@lizziel
Copy link

lizziel commented Nov 2, 2023

I cherry-picked the suggested commit. Thank you @gold2718! I only had to make one minor bug fix despite your lack of testing. :)

@lizziel lizziel closed this Nov 2, 2023
@gold2718 gold2718 deleted the fix_copy branch March 15, 2024 08:53
@gold2718 gold2718 restored the fix_copy branch March 15, 2024 10:11
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.

2 participants