Add a customized config parser #344

xylar · 2022-03-29T22:53:16Z

This config parser can:

keep track of comments from the original config files
keep track of the source of each config option (and include a comment with the source when writing out a combined config file)
prioritize "user" config options over all others

closes #247

xylar · 2022-03-29T22:56:24Z

Example result:

Here is part of the config file for ocean/baroclinic_channel/10km/default as an example:

# Options related to the current test case
[test_case]

# source: /home/xylar/code/compass/customize_config_parser/compass/setup.py
steps_to_run = initial_state forward


# Options related to downloading files
[download]

# the base url for the server from which meshes, initial conditions, and other
# data sets can be downloaded
# source: /home/xylar/code/compass/customize_config_parser/compass/default.cfg
server_base_url = https://web.lcrc.anl.gov/public/e3sm/mpas_standalonedata

# whether to download files during setup that have not been cached locally
# source: /home/xylar/code/compass/compass/inej.cfg
download = True

# whether to check the size of files that have been downloaded to make sure
# they are the right size
# source: /home/xylar/code/compass/compass/inej.cfg
check_size = False

# whether to verify SSL certificates for HTTPS requests
# source: /home/xylar/code/compass/customize_config_parser/compass/default.cfg
verify = True

# the path on the server for MPAS-Ocean
# source: /home/xylar/code/compass/customize_config_parser/compass/ocean/ocean.cfg
core_path = mpas-ocean


# The parallel section describes options related to running tests in parallel
[parallel]

# the program to use for graph partitioning
# source: /home/xylar/code/compass/customize_config_parser/compass/default.cfg
partition_executable = gpmetis

# parallel system of execution: slurm or single_node
# source: /home/xylar/code/compass/compass/inej.cfg
system = single_node

# whether to use mpirun or srun to run the model
# source: /home/xylar/code/compass/compass/inej.cfg
parallel_executable = mpirun

# cores per node on the machine
# source: /home/xylar/code/compass/compass/inej.cfg
cores_per_node = 8

# the number of multiprocessing or dask threads to use
# source: /home/xylar/code/compass/compass/inej.cfg
threads = 8
...

xylar · 2022-03-31T18:44:28Z

@matthewhoffman, this is now ready to review if you would have time.

I tested the functionality I needed for QU meshes in #246 and it worked great! The user config options take precedence over those defined anywhere else so that you can compute other config options based on existing ones anytime you like without having to worry that you might either not yet know or accidentally overwrite what the user has requested.

xylar · 2022-03-31T18:46:42Z

compass/config.py

+            self._combine()
+        return self._combined.has_option(section, option)
+
+    def set(self, section, option, value=None, comment=''):


@matthewhoffman, I haven't yet used the comment argument anywhere in the code. I'd like to add a few examples both to make sure it works as expected and because I think config options that are created in the code (as opposed to just having their previous values changed) should ideally have a comment in the resulting file.

Okay, I've now done this (and fixed a bug related to it).

xylar · 2022-03-31T20:57:02Z

Testing

I set up a few test cases on my Linux laptop to test the user config file and could verify that I saw the user config options in the resulting config files. I also tested various QU resolutions in #246 with this funcationality.

I set up and ran the pr test suite in Anvil (with Intel/Intel-MPI) using the current compass master as a baseline. All tests passed (suggesting that config options remain the same).

I looked a sampling of config file from test cases in the pr test suite and the commit messages look as expected, including for comments passed in as an argument to config.set().

matthewhoffman

Xylar, I looked this over and tested it out, and it looks very useful! I had actually never used a user config file, so this was a good excuse to learn how that works, and that was useful for me to understand. I made a few small comments, nothing major. I marked 'request changes' only because I'd like to see the commenting feature documented in the docs. I don't necessarily need to re-review this though if you're happy with changes you add for that.

matthewhoffman · 2022-04-12T17:47:46Z

compass/setup.py

 import os
 import pickle
 import warnings

-from mache import MachineInfo, discover_machine


Is this change (and related deletions below at ~70 and beyond) just incidental cleanup unrelated to this PR?

No, this is not just clean-up, though I can understand how it might look that way. Before, we were merging a ConfigParser object from mache with one from compass. This previous approach has the advantage that it includes some config options set at runtime in mache such as the username. But it doesn't work with the new approach and it isn't important for compass. So I switched to parsing the config files out of mache instead of using the ConfigParser object, which means that the MachineInfo object is no longer needed.

matthewhoffman · 2022-04-12T17:48:38Z

compass/setup.py

@@ -195,10 +188,6 @@ def setup_case(path, test_case, config_file, machine, machine_info, work_dir,
        The name of one of the machines with defined config options, which can
        be listed with ``compass list --machines``

-    machine_info : mache.MachineInfo


Related to previous comment, why are you able to drop this argument now?

See previous comment.

matthewhoffman · 2022-04-12T17:49:45Z

compass/setup.py

@@ -251,20 +243,10 @@ def setup_case(path, test_case, config_file, machine, machine_info, work_dir,
    test_case.work_dir = test_case_dir
    test_case.base_work_dir = work_dir

-    # add the custom config file once before calling configure() in case we


Out of curiosity, how are you able to drop these config reads?

The design of the new meta-config-parser is that it always prioritizes user config options by parsing them last in the _combine() method. This is much more robust and (I think) more intuitive than the approach used previously (where we added the user config file twice).

matthewhoffman · 2022-04-12T17:53:36Z

docs/developers_guide/framework.rst

+    ...
+
+But it does not allow assignment of a section or many of the other
+dictionary-like features supported by :py:class:`configparser.ConfigParser`.


This is because the CompassConfigParser primarily needs to read configs and not write them, correct? I.e. you only wrapped the configParser functionality we actually need.

Yes, that's exactly right. I only implemented the __getitem__ method and not the other dictionary methods. They would potentially be complicated to implement because of the comment and provenance handling, and I don't foresee a need.

compass/config.py

docs/developers_guide/framework.rst

xylar · 2022-04-13T09:29:36Z

@matthewhoffman, thank you for an excellent review. I have addressed all of your comments and I think the documentation is much improved.

I also added a new user argument to the set() method that is used only in one place in the code right now: to set the path to the MPAS model when it is given on the command line. I thought I would mention this because it wasn't part of your review. I have tested it out and it works correctly: When I put garbage for that config option in my user config file but the right value on the command line, things work as expected.

xylar · 2022-04-13T09:30:51Z

I reran the nightly test suite with my latest changes on my laptop (Linux with Gnu and MPICH) and everything worked fine.

This is a "meta" config parser that keeps a dictionary of config parsers and their sources to combine when needed. The custom config parser allows provenance of the source of different config options and allows the "user" config options to always take precedence over other config options (even if they are added later).

This is needed to set user config options that come from the command-line and not from config files (e.g. the MPAS path)

xylar added in progress This PR is not ready for review or merging python package DEPRECATED: PRs and Issues involving the python package (master branch) framework labels Mar 29, 2022

xylar self-assigned this Mar 29, 2022

xylar mentioned this pull request Mar 29, 2022

Add a general QU mesh to global_ocean #246

Closed

xylar force-pushed the customize_config_parser branch from 9799d70 to e77a1d5 Compare March 31, 2022 15:37

xylar removed the in progress This PR is not ready for review or merging label Mar 31, 2022

xylar marked this pull request as ready for review March 31, 2022 18:41

xylar requested a review from matthewhoffman March 31, 2022 18:41

xylar commented Mar 31, 2022

View reviewed changes

xylar removed the python package DEPRECATED: PRs and Issues involving the python package (master branch) label Apr 1, 2022

xylar force-pushed the customize_config_parser branch 2 times, most recently from 2751905 to 391afc1 Compare April 4, 2022 20:06

xylar mentioned this pull request Apr 5, 2022

Add tests for spherical harmonic transforms #348

Merged

matthewhoffman requested changes Apr 12, 2022

View reviewed changes

xylar force-pushed the customize_config_parser branch from 391afc1 to a083f09 Compare April 13, 2022 09:27

xylar force-pushed the customize_config_parser branch from a083f09 to b9b6b60 Compare April 13, 2022 09:39

xylar added 7 commits April 13, 2022 11:44

Update config parsing in framework

7fcbbce

Update config parsing in ocean framework

af6f4ba

Update config parsing in ocean test groups

bcaaae2

Update config parsing in landice test groups

28b4cc2

Parse config lists with getlist

7ae8d24

Update the docs on config options

211fb59

xylar added 8 commits April 13, 2022 11:44

Add source provenance when writing config files

8da92b0

Add parse and write out config file comments

3dcce4b

Fix comments in set() method

100da75

Add a few example comments in calls to config.set()

9919abf

Fix mache.machines package name

fbbc2f8

Fix set() without comments

49aa938

Add a way to set user config options

631af59

This is needed to set user config options that come from the command-line and not from config files (e.g. the MPAS path)

Set the MPAS path as a "user" config option

a72aed6

xylar force-pushed the customize_config_parser branch from b9b6b60 to a72aed6 Compare April 13, 2022 09:44

xylar merged commit 2120fb9 into MPAS-Dev:master Apr 13, 2022

xylar deleted the customize_config_parser branch April 13, 2022 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a customized config parser #344

Add a customized config parser #344

xylar commented Mar 29, 2022

xylar commented Mar 29, 2022

xylar commented Mar 31, 2022

xylar Mar 31, 2022

xylar Mar 31, 2022

xylar commented Mar 31, 2022

matthewhoffman left a comment

matthewhoffman Apr 12, 2022

xylar Apr 13, 2022

matthewhoffman Apr 12, 2022

xylar Apr 13, 2022

matthewhoffman Apr 12, 2022

xylar Apr 13, 2022

matthewhoffman Apr 12, 2022

xylar Apr 13, 2022

xylar commented Apr 13, 2022

xylar commented Apr 13, 2022

Add a customized config parser #344

Add a customized config parser #344

Conversation

xylar commented Mar 29, 2022

xylar commented Mar 29, 2022

Example result:

xylar commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xylar commented Mar 31, 2022

Testing

matthewhoffman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xylar commented Apr 13, 2022

xylar commented Apr 13, 2022