ESMCI / cime

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

Feature: Parameters for SystemTests #4592

Open jasonb5 opened 5 months ago

jasonb5 commented 5 months ago

A request has come to update the MVK to support another component. This update involves adding additional static values to the MVK test. Continuing to define these additional values is not sustainable especially as more components may utilize the MVK test.

I'm proposing to add a general feature to enable passing a parameter file to create_test e.g. create_test --params /somepath/eamxx_v1.json. In addition we could enable this for testmods by providing a params.json file under the specific testmod directory. An optional level of providing these parameters could be through $SRCROOT/cime_config/test_params/MVK.f19_g16.S.json, this could provide parameters at a specific test level regardless of the testmods.

This feature would be enabled for all test types.

I'm at a toss up for the file format, I was leaning towards a json but I'm not opposed to just defining another XML class.

Example parameter file, if a testmod is used the user_nl* could be omitted.

{
  "component": "eamxx",
  "user_nl": {
    "new_random": ".true.",
    "pertlim": "1.0e-10",
  }
  # could be per inst
  "user_nl_0001": {
    "new_random": ".true.",
    "pertlim": "1.0e-10",
  },
  "module": "ks.py",
  "test-case": "Test",
  "critical": "13",
}

@jgfouca @rljacob @jedwards4b @billsacks

billsacks commented 5 months ago

Can you describe the advantages of this over the current testmods approach?

jasonb5 commented 5 months ago

Testmods can provide the namelist files but it currently cannot provide configuration to the actual test e.g. for MVK configure the invocation of evv4esm or change the component. The addition of the flag to create_test would allow one-offs or overriding the testmods values.

jgfouca commented 5 months ago

One good thing about testmods is that using them changes the case name which is important since the mods make it a different test than unmodded. Why not just incorporate JSON support into the testmod system? Kind of like how it expects shell_commands to be there now, it could also check for .json files.

jasonb5 commented 5 months ago

I'm fine making testmods the default way of providing the parameters.

Is it worth including a new argument to create_test to override testmods? A use-case I was thinking of is a user wanting to test something without changing the testmod, maybe this isn't really a practical use-case though.

billsacks commented 5 months ago

I like the idea of extending the testmod system to support JSON that could configure the test. I agree with @jgfouca that the incorporation of the testmod in the test name is an important feature, and I think that providing a way to override it without changing the test name is asking for trouble. You can create a new testmod directory if you need to override something. But I don't think I totally understand the use case that you're thinking about @jasonb5 so maybe there's some value in there that I'm missing.

rljacob commented 5 months ago

This PR has an attempt to add MVK to the ocean https://github.com/E3SM-Project/E3SM/pull/6207 But it seems to be using the xml test system which E3SM doesn't use. Can what you propose allow MVK to be used for ocean?

rljacob commented 5 months ago

@mkstratos

jasonb5 commented 5 months ago

@billsacks You're right, it's simple enough to change the file or create another testmod so there's probably no merit in adding it.

jasonb5 commented 5 months ago

@rljacob I think we could support https://github.com/E3SM-Project/E3SM/pull/6207 using this feature. For the more dynamic tasks we could enable a hook system in the test and allow a params.py file. Alternatively we could just ditch the .json and get away with just a params.py.

Might look something like this.


def perturb_init(infile, field_name, outfile, seed=None):
  ...

def modify_stream(stream_file, input_file=None, var_names=None, output_stream_name="timeSeriesStatsClimatologOutput"):
  ...

def generate_nml_files(case, ...):
  ...

def get_evv4esm_config(case, ...):
   config = {...}
   ...
   return config
jedwards4b commented 4 months ago

I am looking at this test for the first time and I realize that I have some concerns with the way that it is implemented. First, as far as I can tell, this test only works with the CAM model as defined in e3sm. There are no namelist variables for new_random, seed_custom and seed_clock in CAM as defined in CESM.

So if this change would make the test useful for CESM by removing the namelist modifications to a json file whose selection could depend on CIME_MODEL then I support the change. I also agree that it is important to maintain a one-to-one correspondence between test names and the actual test conducted, so doing this as a part of the testmods is also good. I would also like to point out that this reference: https://github.com/E3SM-Project/E3SM/blob/master/cime/scripts/climate_reproducibility/README.md from mvk.py appears to be a broken link.

jasonb5 commented 4 months ago

@jedwards4b That's correct this update would make the mvk test completely customizable through a testmod. Do you have a preference between a python module or json file? I'm leaning towards a python module so we can support more complex setup for the test as required by https://github.com/E3SM-Project/E3SM/pull/6207. We could also support a json file for a simple configuration this wouldn't add too much.

github-actions[bot] commented 1 month ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 5 days.