zppy-interfaces refactor#642
Conversation
forsyth2
left a comment
forsyth2
left a comment
There was a problem hiding this comment.
@xylar @chengzhuzhang Following up from E3SM-Project/zppy-interfaces#1 (review), the major changes on the zppy side so far are:
- Better organization of the
zppy/templatesdirectory:.bashfiles remain inzppy/templates.- Defaults (
default.ini, campaigncfgs) are now inzppy/defaults. - The
global_time_series.pyfiles have been moved tozppy-interfaces/global-time-series, outside of this repo entirely. e3sm_diags/lat_lon_conus.cfgdidn't appear to be used anywhere, so I createdzppy/examplesto house that.- Some files are included into the bash templates, so I've placed these in
zppy/templates/inclusions
- As mentioned in E3SM-Project/zppy-interfaces#1 (comment), the
.bashtemplate for Global Time Series has been greatly simplified by moving much of that code over to Python inzppy-interfaces.
| exit 6 | ||
| fi | ||
|
|
||
| zppy-interfaces global-time-series --use_ocn={{ use_ocn }} --global_ts_dir={{ global_time_series_dir }} --input={{ input }} --input_subdir={{ input_subdir }} --moc_file={{ moc_file }} --case_dir={{ output }} --experiment_name={{ experiment_name }} --figstr={{ figstr }} --color={{ color }} --ts_num_years={{ ts_num_years }} --plots_original={{ plots_original }} --atmosphere_only={{ atmosphere_only }} --plots_atm={{ plots_atm }} --plots_ice={{ plots_ice }} --plots_lnd={{ plots_lnd }} --plots_ocn={{ plots_ocn }} --regions={{ regions }} --start_yr={{ year1 }} --end_yr={{ year2 }} |
There was a problem hiding this comment.
I like your idea of a config file instead of this. It would be good to break the command into multiple lines with \ for better readability if you do keep this approach.
|
Yep, so far, I like the changes you're proposing! |
xylar
left a comment
xylar
left a comment
There was a problem hiding this comment.
This looks good to me! Just a few comments that don't need addressing in this PR.
| @@ -0,0 +1,67 @@ | |||
| """Logger module for setting up a logger. Based on xcdat/xcdat/_logger.py""" | |||
There was a problem hiding this comment.
This is redundant with https://github.com/E3SM-Project/zppy-interfaces/blob/initial-implementation/zppy_interfaces/multi_utils/logger.py but I think that's pretty hard to avoid.
Still, worth keeping an eye on this kind of redundancy and having a backup plan if it gets bad.
|
|
||
| [global_time_series] | ||
| active = True | ||
| environment_commands = "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate zppy_interfaces_dev_pr_1_20241121v4" |
There was a problem hiding this comment.
Presumably, this will be removed once there are shared environments that support zppy-interfaces?
There was a problem hiding this comment.
Well, realistically this will remain because we'll always want to test zppy with the latest zppy-interfaces code, not whatever the last release was. But yes, for cfgs written by users they could rely on Unified having zppy-interfaces in it.
There was a problem hiding this comment.
The problem I see is that this environment is only on Chrysalis (or Anvil). What if you want to test somewhere else?
There was a problem hiding this comment.
Or is this just a placeholder?
There was a problem hiding this comment.
Oh, that's the responsibility of tests/integration/utils.py. This file is auto-generated by that one. I currently only have a Chrysalis environment set up, so it's the only one defined.
There was a problem hiding this comment.
I also think it is a little odd to have machine and user specific path here, but don't know how to get around.
There was a problem hiding this comment.
Well we could use $USER to get my name out of it but ultimately whoever runs this cfg will need to create their own conda environment for the latest developments or else use Unified.
I think maybe the best solution is to always keep the environment_commands lines on the main branch filled in as <path to YOUR conda>; conda activate <environment YOU create>.
There was a problem hiding this comment.
Ok, the commit dbd74f3 implements the idea I mentioned in the previous comment.
There was a problem hiding this comment.
It seems like it would be better to have some examples somewhere but not to update them very often. This is the approach we use in MPAS-Analysis.
There was a problem hiding this comment.
The problem with that is I fear they will very quickly go out-of-date. This approach (the auto-generation of scripts from templates) updates everything together.
@forsyth2 this .cfg file is actually referenced here: https://github.com/E3SM-Project/zppy/blob/408e9ade87db8d0d5322c01186564df1392282be/docs/source/post.rrm_simulation.cfg#L59C1-L62C39 |
|
@chengzhuzhang Ah thank you. Ok, so this should actually be included under |
|
@chengzhuzhang The commit be06191 addresses your comment. |
3 participants
Issue resolution