Removing ndsl.Namelist from PyFV3

[jjuyeonkim]

Description
This follows from NDSL PR #246 and is part of NDSL Issue # 64.

This PR attempts to remove ndsl.Namelist from PyFV3 by doing the following:

• ModifiesDynamicalCoreConfig.from_f90nml() to use only f90nml.Namelist via NDSL helper functions
• Removes DynamicalCoreConfig.from_namelist()
• Modifies the translate tests to shed its reliance on ndsl.Namelist in favor the DynamicalCoreConfig
• Adds --no_legacy_namelist to the github workflow translate.yaml

Partial work on NDSL Issue # 64

How Has This Been Tested?

• Existing unit tests
• One new simple unit test that checks out creating the DynamicalCoreConfig from a nml file with a "namelist_override"

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules - There is a related Pace PR that needs to be checked in first for the pace tests to pass: Temporary Dycore dacite Config + Dycore Config Type Check Tweak [see: PyFV3 PR #88] pace#152
• New check tests, if applicable, are included
• Targeted model if this changed was triggered by a model need/shortcoming

[romanc]

Thanks, Janice. That looks like a step in the right direction to me. I agree that DynamicalCoreConfig.from_namelist() should go away. And I think it's smart to store the dycore config in test classes as self.config.

I'm curious to hear why you decided to have dycore_config_from_f90nml as a standalone function instead of updating DynamicalCoreConfig.from_f90nml(). It seems to me that what ever is done in dycore_config_from_f90nml() could just move into DynamicalCoreConfig.from_f90nml(), no? Maybe I'm missing something.

[jjuyeonkim]

I'm curious to hear why you decided to have dycore_config_from_f90nml as a standalone function instead of updating DynamicalCoreConfig.from_f90nml(). It seems to me that what ever is done in dycore_config_from_f90nml() could just move into DynamicalCoreConfig.from_f90nml(), no? Maybe I'm missing something.

You bring up a good point on keeping DynamicalCoreConfig.from_f90nml(). This was likely a misunderstanding on my part. I added it back.

There's also the fact that DynamicalCoreConfig.from_yaml() exists as well. So, keeping the from_f90nml() would just keep following that convention.

[jjuyeonkim]

This is a really simple test for trying out the override_namelist. I didn't see one that tested it out, so I didn't think it would hurt.

However:

1. I don't know if it goes here or in the Pace unit tests. It's not currently being run during the CI because there's another test in the same directory that also isn't being run and may be outdated.
2. I'm hoping I got the functionality of the test right -- that the namelist_override completely overrides the previous namelist?

If no one knows, I'll just remove the test.

Edit: Ultimately, i just removed the unit test for now.

[jjuyeonkim]

I changed a few things:

• I added a member variable to the DynamicalCoreConfig to keep track of the namelist groups for a particular instance:
  target_nml_groups: Optional[Tuple[str, ...]] = DEFAULT_DYCORE_NML_GROUPS
  i don't know if it's overkill. I added it to keep track of the namelist groups of a particular instance. The handling of the namelist_override happens in the post init, so in theory it could be initialized from a yaml or from dict or anywhere like that.
• I added a new from_dict function that's used through from_f90nml. I figured this would be useful in the pace driver eventually, when initializing the dycore config from the driver config. I added some additional parameters. There's a dacite config that I'm using to work with the tuples. Right now, I'm making it strict=False because we're basically just flattening namelists from certain groups, so there's a chance that additional parameters from those particular namelist groups will be passed on to the dycore config.
• There's a current work-around in pace related to one of the unit tests as a result of my changes: Pace PR #152 . I believe the pace CI test that's failing now will pass with this work-around.

[romanc]

Thanks, Janice! I think we are moving the right direction. A couple of smaller things inline.

I think some tests will break because the don't automatically get self.config added. The class hierarchy for translate tests is a mess (and I'm not sure we get to clean it up anytime soon).

And while we're at it, I'd suggest to take TranslateDycoreFortranData2Py out of translate_fvdynamics.py because the class isn't used there. I'd suggest to put it in a separate file in the same folder and then adjust the path in the __init__.py such that imports like "from pyfv3.testing import TranslateDycoreFortranData2Py" don't need any change.

[romanc]

Suggested change

	) -> "DynamicalCoreConfig":
	) -> DynamicalCoreConfig:

You can add from __future__ import annotations at the top of the file and then you don't need to add quotes around types any more that the old type resolver couldn't resolve

[jjuyeonkim]

I'll update this.

Edit -- NOTE: I made a slight tweak to Pace's tests/main/fv3core/test_config.py::test_types_match test to account for this change.

[jjuyeonkim]

Okay, I tried to move TranslateDycoreFortranData2Py into its own translate_data.py

[romanc]

Thanks, Janice. From my point of view, this is good to go 🚀