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

Plugins for zppy #402

Merged
merged 5 commits into from
Jun 8, 2023
Merged

Plugins for zppy #402

merged 5 commits into from
Jun 8, 2023

Conversation

golaz
Copy link
Collaborator

@golaz golaz commented Feb 9, 2023

Plugins for zppy

A number of users have expressed interest in adding new functionality to zppy. Until now, the process of adding new tasks to zppy required directly modifying the zppy source code to add the appropriate hooks in the python code and add new Jinja2 templates. This is unnecessarily complicated and could cause the zppy code base to become bloated over time with code that should live in a separate repository. The goal of zppy is to streamline the workflow, not to directly incorporate tools and diagnostics.

The new zppy plugin functionality aims to solve these issues by providing a very simple mechanism for users to call custom python code without any modifications to the zppy source code. The plugin is maintained in a separate location, and the user only needs to modify the zppy configuration file:

[default]
plugins = "/pathToMyGreatPlugin/myGreatPlugin.py" # list of one or more external plugins

# Add a new section for each external plugin

[myGreatPlugin]
# Needed onfiguration options go here...

Minimum directory structure of the plugin code:

myGreatPlugin.py    # main code with a function 'myGreatPlugin' that will be called by zppy
templates/
   # all the Jinja2 templates needed

Over time, we envision that certain user-created plugins will mature and gain sufficient adoption to become official zppy plugins. At that point, the plugin source code should be included into E3SM Unified and zppy will be modified to know its location without having to specify it explicitly in the configuration file. But testing and ensuring compatibility with new zppy releases will remain the responsibility of the plugin maintainer.

@golaz golaz marked this pull request as draft February 9, 2023 01:16
@forsyth2
Copy link
Collaborator

forsyth2 commented Feb 9, 2023

This will be great, thanks @golaz!

@golaz
Copy link
Collaborator Author

golaz commented Feb 9, 2023

A few more ideas for consideration

  1. Implement mechanism to validate and provide default values for [sections] in configuration file that correspond to plugin. The user plugin would have to provide its own default.ini.
  2. In [default], plugins variable could point to a configuration file (json, yaml) instead of the actual plugin python file. This would provide more flexibility for naming convention and possibilities for future expansion.

@chengzhuzhang
Copy link
Collaborator

chengzhuzhang commented Feb 9, 2023

@golaz I'm very excited about this new feature! it can make zppy more extensible and effective to serve as an interface for needed tools.
I feel it might be useful to have a cookiecutter python package repository setup as a template for meeting requirement to be included into E3SM-unified (i.g a conda release) . @xylar is setting up a new repo following one example set up by@tomvothecoder. I think a more minimum example will be useful for each plugin maintainers.

zppy/__main__.py Outdated
Comment on lines 192 to 196
for plugin in user_config["default"]["plugins"]:
# Sanity check: plugin is a file and has .py extension
if not (os.path.isfile(plugin) and plugin.endswith(".py")):
print('WARNING: Skipping invalid external plugin = "%s"' % (plugin))
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

@golaz and @tangq, I don't think providing a path to a plugin is going to be a good approach if you want plugins to become conda packages. The path to the code will be pretty convoluted, machine specific and not easy for a user to figure out. Instead, it would be better if it were a python package with a standard format and you simply gave the name of the python package. (It would be even better the python packages for zppy plugins all started with zppy- because that's a very common convention for python plugins.)

You can use importlib.resources to get file locations and read text from files in python packages. This would be my strong recommendation here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@xylar, thank you for raising those good points.

We could certainly change how plugins are specified. As I was adapting @hsiangheleellnl and @tangq code, I looked for the simplest possible entry option to get users started interfacing their python code. As the development of a plugin matures, the code should become a standard python package. But it might be better to require this from the onset.

I like the idea of names starting with zppy-, not sure if some developers might prefer a standalone name though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it depends on if the package has a life if its own outside of being a zppy plugin. Then, the zppy prefix wouldn't make much sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

We decided on chemdyg to match the repo name for that particular package. No zppy- prefix.

@golaz
Copy link
Collaborator Author

golaz commented Mar 9, 2023

Unresolved: how do zppy plugins provide their dafault.ini settings, and how does zppy handle them.

@xylar
Copy link
Contributor

xylar commented Mar 9, 2023

Unresolved: how do zppy plugins provide their dafault.ini settings, and how does zppy handle them.

I would think you could have a standard filename (probably default.ini) that you look for in the root of each plugin package. Again, you can use importlib.resources to determine if there is such a file in the package and read it if so.

@tangq
Copy link
Contributor

tangq commented Mar 15, 2023

I think it is a good idea to have chemdyg's own default.ini if it is possible. As of now, all the chemdyg settings are in the cfg e3sm_chem_diags (will be renamed to chemdyg) session in this example.

@xylar
Copy link
Contributor

xylar commented Mar 16, 2023

@golaz, how will zppy's internally set config options be passed on to plugins? In particular, I would like to avoid duplicating this kind of code in the plugin if possible:

zppy/zppy/__main__.py

Lines 77 to 102 in c3f70dc

if ("machine" not in config["default"]) or (config["default"]["machine"] == ""):
# MachineInfo below will then call `discover_machine()`,
# which only works on log-in nodes.
machine = None
else:
# If `machine` is set, then MachineInfo can bypass the
# `discover_machine()` function.
machine = config["default"]["machine"]
machine_info = MachineInfo(machine=machine)
default_machine = machine_info.machine
(
default_account,
default_partition,
default_constraint,
_,
) = machine_info.get_account_defaults()
unified_base = machine_info.config.get("e3sm_unified", "base_path")
config["default"]["diagnostics_base_path"] = machine_info.config.get(
"diagnostics", "base_path"
)
config["default"]["web_portal_base_path"] = machine_info.config.get(
"web_portal", "base_path"
)
config["default"]["web_portal_base_url"] = machine_info.config.get(
"web_portal", "base_url"
)

Is there a way that zppy can pass the config object when it calls the plugin?

@xylar
Copy link
Contributor

xylar commented Mar 16, 2023

Sorry, silly question. This is clearly the case looking at the code.

@forsyth2
Copy link
Collaborator

@golaz Here's my commit of fixes you'll need: 3b2338e

I ran the integration tests. I had to add in a if "plugins" in user_config["default"].keys(): line to get zppy to run at all. Other than that, they look to be good in shape. The test_bash_generation, test_campaign, and test_defaults integration tests will need to have their expected results updated after merging (e.g., https://github.com/E3SM-Project/zppy/blob/main/tests/integration/generated/directions_chrysalis.md#commands-to-run-to-replace-outdated-expected-files).

The unit tests pass with the fixes in the commit.

I also ran the pre-commit checks and made two changes to get those passing.

@golaz golaz changed the title [WIP] Plugins for zppy Plugins for zppy Jun 8, 2023
@golaz golaz marked this pull request as ready for review June 8, 2023 23:16
@golaz golaz requested a review from forsyth2 June 8, 2023 23:17
@golaz golaz merged commit a3c64e6 into main Jun 8, 2023
4 checks passed
@golaz golaz deleted the golaz/plugins branch June 8, 2023 23:20
@forsyth2
Copy link
Collaborator

forsyth2 commented Jun 9, 2023

I updated expected files for the integration tests, so it looks like those are all passing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants