[Bug]: Another parameter checking error: zppy.utils.ParameterNotProvidedError: climo_land_subsection

[chengzhuzhang]

What happened?

I think it is also something introduced in v3. My config is as follows:

[e3sm_diags]
active = True
walltime = "4:00:00"
#years = "0001:0100:50", "0001:0100:100"
years = "1985:2014:30",
ts_num_years = 30
ref_start_yr = 1985
ref_final_yr = 2014
multiprocessing = True
num_workers = 8

  [[ lnd_monthly_mvm_lnd ]]
  # Test model-vs-model using the same files as the reference
  #environment_commands = "source /home/ac.zhang40/y/etc/profile.d/conda.sh; conda activate edv290"
  grid = 'native'
  climo_subsection = "land_monthly_climo"
  diff_title = "Difference"
  partition = "compute"
  qos = "regular"
  short_name = v3.LR.piControl
  ref_name = "20231209.v3.LR.piControl-spinup.chrysalis"
  ref_start_yr = 0051
  ref_final_yr = 0100
  ref_years = "0051-0100",
  reference_data_path = "/lcrc/group/e3sm/ac.zhang40/tests/20231209.v3.LR.piControl-spinup.chrysalis_land_diags/post/lnd/native/clim"
  run_type = "model_vs_model"
  sets = "lat_lon_land",
  short_ref_name = "20231209.v3.LR.piControl-spinup"
  swap_test_ref = False
  tag = "model_vs_model"
  ts_num_years_ref = 50

URL: https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.zhang40/E3SMv3_eurc5/v3.LR.historical_0051/e3sm_diags
e3sm_diags_lnd_monthly_mvm_lnd_model_vs_model_1985-2014_vs_0051-0100
Traceback (most recent call last):
  File "/home/ac.zhang40/.conda/envs/zppyv3/bin/zppy", line 8, in <module>
    sys.exit(main())
  File "/home/ac.zhang40/.conda/envs/zppyv3/lib/python3.9/site-packages/zppy/__main__.py", line 61, in main
    _launch_scripts(config, script_dir, job_ids_file, plugins)
  File "/home/ac.zhang40/.conda/envs/zppyv3/lib/python3.9/site-packages/zppy/__main__.py", line 237, in _launch_scripts
    existing_bundles = e3sm_diags(config, script_dir, existing_bundles, job_ids_file)
  File "/home/ac.zhang40/.conda/envs/zppyv3/lib/python3.9/site-packages/zppy/e3sm_diags.py", line 69, in e3sm_diags
    add_climo_dependencies(c, dependencies, script_dir)
  File "/home/ac.zhang40/.conda/envs/zppyv3/lib/python3.9/site-packages/zppy/e3sm_diags.py", line 264, in add_climo_dependencies
    check_parameter_defined(c, "climo_land_subsection")
  File "/home/ac.zhang40/.conda/envs/zppyv3/lib/python3.9/site-packages/zppy/utils.py", line 323, in check_parameter_defined
    raise ParameterNotProvidedError(relevant_parameter)
zppy.utils.ParameterNotProvidedError: climo_land_subsection

What machine were you running on?

Chrysalis

Environment

e3sm u rc5 with zppy rc2

What command did you run?

zppy -c

Copy your cfg file

see above

What jobs are failing?

What stack trace are you encountering?

[forsyth2]

@chengzhuzhang I'm pretty sure you just need to use climo_land_subsection = "land_monthly_climo" instead of climo_subsection = "land_monthly_climo". The issue was that there were cases where the land diags needed to use a different climo.

I'm going to see about adding some sort of explanation to these ParameterNotProvidedErrors as part of a PR before the next rc.

[chengzhuzhang]

@chengzhuzhang I'm pretty sure you just need to use climo_land_subsection = "land_monthly_climo" instead of climo_subsection = "land_monthly_climo". The issue was that there were cases where the land diags needed to use a different climo.

I'm going to see about adding some sort of explanation to these ParameterNotProvidedErrors as part of a PR before the next rc.

I saw the PR to add explanation, but i think the error message itself is pretty clear. Just I don't understand the change, and the issue was that there were cases where the land diags needed to use a different climo. . I shared my cfgfile cross groups, this is another non backward compatible issue, we need to explain to the users. I would avoid it if this is not necessary.

[forsyth2]

@chengzhuzhang Ah, after looking into the origins of this parameter, I have a few notes:

• climo_land_subsection was added in Update to diags handling #633, specifically addressing [Bug]: Issues with tc_analysis in zppy #622 (comment). It looks like it was added because the land diags run didn't know what climo subsection to use.
• It looks like I followed the example of diurnal cycle (climo_diurnal_subsection, which has been around for years). I believe diurnal cycle is implemented this way because it is not typically run as a separate e3sm_diags subtask (i.e., diurnal_cycle is one set among many in an e3sm_diags run).
• That said, we haven't typically been running lat_lon_land like that -- we've been running it as a separate subtask. And at that point, why not just override the existing climo_subsection with something else, rather than adding a new parameter climo_land_subsection?

So this brings us to the following options for a design decision:

1. Completely replace climo_land_subsection with climo_subsection. Because this task requires a different climo subtask as a dependency, this would mean lat_lon_land could never be run along with other sets! Is that always going to be the case? I was under the impression we were just running it separately while issues were worked out on the e3sm_diags side.
2. Apply the inferring logic of Improve input validation and testing #628 (and Allow e3sm_to_cmip to use ilamb parameter names #672 (comment)). If climo_land_subsection isn't given, assume we're just using the value for climo_subsection.
3. Always require the user to specify climo_land_subsection is using the lat_lon_land set. (main as of today).

[chengzhuzhang]

Can we revert to use climo_subsection instead, without introducing climo_land_subsection?

[forsyth2]

That's option 1. That commits us to always running lat_lon_land as a separate task. lat_lon_land is not going to be able to be included in an e3sm_diags viewer with other sets. If that's not a use case on the horizon, then we can do option 1.

[chengzhuzhang]

That's option 1. That commits us to always running lat_lon_land as a separate task. lat_lon_land is not going to be able to be included in an e3sm_diags viewer with other sets. If that's not a use case on the horizon, then we can do option 1.

Yes, this is reasonable.

[forsyth2]

Ok, resolved in #679

[forsyth2]

I saw the PR to add explanation, but i think the error message itself is pretty clear.

I think the additional explanations in https://github.com/E3SM-Project/zppy/pull/675/files would still be helpful to users, so I think I'm still going to merge that.