Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for shared config options #125

Merged
merged 15 commits into from
Oct 3, 2023

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Sep 28, 2023

This merge adds the capability of sharing a config object across steps and tasks. This allows:

  • shared steps like those for creating spherical meshes to have their own config options
  • multiple tasks to share the same config options

The capability has been made possible by:

  • storing a list of configs in each component
  • the modifications to MpasConfigParser in Add new features to config parser MPAS-Dev/MPAS-Tools#527
  • modifying the PolarisConfigParser to include a filepath where it will be written and a list of symlinks that should be created
  • adding methods to Task and Step for setting a shared config, which perform error checks, determine the paths for symlinks and add the config to the component
  • extensive changes to the setup and run.serial modules to accommodate these other changes

The changes mean that it is now possible to add config files from the polaris package to the config object at initialization, rather than having to use the task's configure() method. Since config objects shared across tasks should ideally not be updated in a given task's configure() method, a setup() method has been added to PolarisConfigParser that can be overridden to perform more complex configuration as needed. So far, this capability is not being used.

Tasks have also been modified to make use of the functionality:

  • Baroclinic channel tasks at a given resolution now all share a baroclinic_channel.cfg file in the resolution directory
  • Shared icos and qu meshes now have their own config options in the shared step's directory
  • Cosine bell task (with and without viz) and steps other than the base mesh all share a single config file. There are separate config files for the icos and qu versions of the tasks and steps.
  • Remaining ocean tasks have been simplified to remove configure() methods that are no longer needed.

This merge requires updating to mpas_tools v0.24.0 to bring in the updates to MpasConfigParser.

Checklist

  • User's Guide has been updated
  • Developer's Guide has been updated
  • API documentation in the Developer's Guide (api.md) has any new or modified class, method and/or functions listed
  • Documentation has been built locally and changes look as expected
  • Testing comment in the PR documents testing used to verify the changes

This is used, among other things, to tell which steps are
shared.
The `filepath` and `symlinks` attributes as well as the `setup()`
method will help support shared config options.
This merge adds a list of `configs` to the component and a
method for adding new ones (or checking for consistency if the
config has already been added).

It also adds `set_shared_config()` methods to both `Task` and
`Step` that can be used to set a shared config parser, possibly
with a local symlink.
@xylar xylar added enhancement New feature or request framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to ocean tests or analysis labels Sep 28, 2023
@xylar xylar requested review from sbrus89 and cbegeman September 28, 2023 09:47
@xylar xylar self-assigned this Sep 28, 2023
@xylar
Copy link
Collaborator Author

xylar commented Sep 28, 2023

Testing

I have successfully run all ocean tasks (except RPE at 1 and 4 km) on Chrysalis with Intel and OpenMPI.

I have verified that config files and symlinks appear in the expected locations, and that they retain the "raw" formatting that allows for extended interpolation (e.g. convergence_eval_time = ${cosine_bell:vel_pd}) upon reading them back in.

@xylar
Copy link
Collaborator Author

xylar commented Sep 28, 2023

@cbegeman and @sbrus89, this is ready to review when you have time. The review can be at whatever level you have time for. The main thing I want to make sure of is that you are good with the changes I'm making at a conceptual level:

  • Does it make sense to you that each step for making a uniform spherical mesh (e.g. ocean/spherical/icos/base_mesh/60km) has its own config options, and that there are shared config options between all the tasks in ocean/planar/cosine_bell/10km and ocean/spherical/icos/cosine_bell?
  • Do the symlinks in each task and step to the relevant config file make it clear (for developers and users alike) what config file is being used?
  • Is the process for making a shared config and adding it to test cases and steps clear enough that you would be able to borrow from the examples (baroclinic channel and cosine bell) to make use of it?

As always, suggestions (from broad to nit-picky) are welcome.

@xylar xylar force-pushed the add-shared-configs branch 3 times, most recently from ff2640f to 0fb0025 Compare September 29, 2023 12:10
@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2023

I'm running into some issues when I try to change resolutions in the cosine bell tests. Please hold off on reviewing this for now.

@xylar xylar added the in progress This PR is not ready for review or merging label Sep 29, 2023
@xylar xylar marked this pull request as draft September 29, 2023 12:23
@xylar xylar force-pushed the add-shared-configs branch from 0fb0025 to 9a32d98 Compare September 29, 2023 13:46
Since config files can now be added in the constructor, we do
this instead.  This will slightly increase load time but likely
not enough to notice.
@xylar xylar force-pushed the add-shared-configs branch from d6103be to a9e6f3c Compare September 29, 2023 17:15
@xylar xylar force-pushed the add-shared-configs branch from a9e6f3c to 130af80 Compare September 30, 2023 05:48
@xylar xylar marked this pull request as ready for review September 30, 2023 05:50
@xylar xylar removed the in progress This PR is not ready for review or merging label Sep 30, 2023
@xylar
Copy link
Collaborator Author

xylar commented Sep 30, 2023

With a few more bug fixes, this now works even when I modify the resolutions for cosine bell in a user config file as part of setting up the test cases.

@xylar
Copy link
Collaborator Author

xylar commented Oct 1, 2023

Okay, I finally got the new-new mpas_tools v0.25.0 working and this is ready for review. @cbegeman and @sbrus89, please take a look when you can. I'll continue to work on branches based off of this one in the meantime.

Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xylar Conceptually and practically, this all looks great! I ran cosine_bell (incl. editing the resolutions in the cfg file at setup) and one of the new sphere_transport cases rebased on this branch.

@xylar
Copy link
Collaborator Author

xylar commented Oct 2, 2023

Great, thanks very much @cbegeman! I appreciate you taking the time to review and test!

@xylar xylar removed the request for review from sbrus89 October 3, 2023 13:28
@xylar
Copy link
Collaborator Author

xylar commented Oct 3, 2023

Since @sbrus89 is away at a workshop this week, I think it would be better to get this merged now with @cbegeman's review. Other work is building on this so it is a little bit urgent.

@xylar xylar merged commit a702eeb into E3SM-Project:main Oct 3, 2023
@xylar xylar deleted the add-shared-configs branch October 3, 2023 13:29
@sbrus89
Copy link
Contributor

sbrus89 commented Oct 6, 2023

Sorry I didn't get to this sooner @xylar and @cbegeman.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants