Namelist Refactor: Utility functions for namelist to dict + conftest --no_legacy_namelist

[jjuyeonkim]

Description

Partial work addressing issue #64

Adding helper functions for loading f90nml.Namelist from file and converting to dict.

There is a slight complication with the translate tests. We need to be able to turn on and off the usage of ndsl.Namelists. So, I'm proposing a new temporary flag --no_legacy_namelist in conftest.py to toggle between two options in the tests:

1. Use only f90nml.Namelist (Long-term desired goal) within the translate tests
2. Continue using f90nml.Namelist and the soon-to-be legacy ndsl.Namelist (Interim solution to facilitate checking in changes one submodule at a time)

Also, I'm modifying ParallelTranslate's layout() to rely more on f90nml.Namelist or ndsl.Namelist temporarily as we work on this issue.

These changes will facilitate the eventual removal of ndsl.Namelist in PyFV3 and PySHiELD in subsequent PRs.

Fixes #64 (partially)

How has this been tested?

Existing unit tests still pass and translate tests pass as expected when compared to the develop branch.

(I have also a proposed set of changes in PyFV3 that use these modifications to get rid of ndsl.Namelist, where the tests all pass as expected. I can hopefully put together a PR relatively quickly if this PR's changes look good.)

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
• New check tests, if applicable, are included

[FlorianDeconinck]

Looking good - I'll have a deeper look later, for now some small comments on tracking temporary changes

[FlorianDeconinck]

Parts of me would like to hide f90nml behind a "universal" configuration loader. But this is a good first step in any case.

Thanks for the pathlib cleanup.

[jjuyeonkim]

Parts of me would like to hide f90nml behind a "universal" configuration loader. But this is a good first step in any case.

I think I understand and agree with this sentiment regarding a more universal loader in the future (without the f90nml explicitly being there). I personally don't know exactly what that looks like yet, so I thought that getting rid of the ndsl Namelist and NamelistDefault would be a good place to help untangle things and get us closer.

For a future PR, having something like a 'load_config' that handles both yaml and namelist sounds like a good goal. Maybe pursuing that idea of mapping from f90nml to yaml at that point.

[FlorianDeconinck]

Parts of me would like to hide f90nml behind a "universal" configuration loader. But this is a good first step in any case.

I think I understand and agree with this sentiment regarding a more universal loader in the future (without the f90nml explicitly being there). I personally don't know exactly what that looks like yet, so I thought that getting rid of the ndsl Namelist and NamelistDefault would be a good place to help untangle things and get us closer.

For a future PR, having something like a 'load_config' that handles both yaml and namelist sounds like a good goal. Maybe pursuing that idea of mapping from f90nml to yaml at that point.

Arguably we should wait to encounter a third file schema we need to carry to generalize for good in order to not pre-engineer code. I am sure someone will come along with some RC file format from outer hell for us :D

[romanc]

This looks like a good start to me. Something bigger is definitely bubbling up here, but let's take it step by step. I love the push towards pathlib, it's a nice addition to this cleanup.

I'd like to discuss / change the new pytest flag and avoid putting an explicit reference to f90nml in there. I think the format of the config file should really be an implementation detail. The proposed name --legacy_namelist_support is up for debate. I'm happy to hear different opinions.

[romanc]

It seems to me like these "grid params" could be a type (instead of an un-typed, general dict). Maybe that's a good follow-up PR or just something to think about. Might be out of scope for this PR.

[FlorianDeconinck]

I think this is full on config dataclass in a follow up PR. E.g. something like:

class CartesianGridParameters
    nx: int,
    ny: int,
    nz: int,
    
class CubeSphereGridParameters:
   layout: tuple[int, int]
   cartesian_dims: CartesianGridParameters

[jjuyeonkim]

Sounds like a good follow-up. I'll put in a TODO. Also, if this PR gets approved, I'll create an issue to keep track.

[jjuyeonkim]

#253

[romanc]

Sorry for being late and for pulling this out of the queue. I'd really like to talk about the name of the new pytest flag.

[romanc]

Looking good now 🚀