[WIP]: Support for command-line plugins #6691

wihobbs · 2025-03-07T01:15:55Z

This is being put up as WIP so that people can start to comment on the proposed design and point out testing gaps.

This is built off of @grondo's cli-plugins branch, with the modification that plugins should be namespaced under a -P flag, similar to how clang's -Wl flag works but more along the lines of -o/-S "plus" in flux. Here you get -P KEY[=VAL] but with plugin validation and ability for args to modify jobspecs and config files. It also incorporates the suggestion for a FLUX_CLI_PLUGINPATH environment variable that overrides the standard path in confdir for testing.

I'll fold up the outstanding TODOs here, most are documented in the comments:

Utilize the argparse.HelpFormatter class to create a better-formatted argument group for the plugins
Removing the superfluous base classes from self.plugins in the CLIPluginRegistry
The validate callback for bar.py seems to be doing nothing; probably misunderstanding how the interface for the validator is supposed to work

I have been staring at this all day and my brain is toast, so apologies if some very basic things are missing or out of place.

Problem: There's no way to extend job submission cli commands in Flux via plugins. Add a prototype CLIPlugin class and CLIPluginRegistry (which loads CLIPlugins).

Problem: Flux submission cli commands don't support plugins. Load any plugins in the default plugin path "{confdir}/cli/plugins/*.py" and call any plugin `add_options()` and `modify_jobspec()` callbacks at appropriate points.

Problem: flux.job.Jobspec.validate_jobspec() throws away the parsed jobspec object if validation is successful. However, it could be useful to return the resulting jobspec object. Return a tuple of (result, jobspec) from validate_jobspec().

Problem: Job submission cli plugins offer a validate() callback, but this callback is not used. Load plugins and run all validate callbacks in the jobspec validator plugin.

Problem: the CLI plugin interface has no testing. Add some.

src/bindings/python/flux/cli/base.py

@@ -36,6 +36,7 @@

 import flux
 from flux import debugged, job, util
+from flux.cli.plugin import CLIPluginRegistry, CLIPluginValue


grondo

I took a quick pass this evening. I haven't tested yet nor looked into why the validator isn't working. I'm not sure if I tested that in the previous prototype.

grondo · 2025-03-07T01:34:42Z

src/bindings/python/flux/cli/plugin.py

+    def __init__(self, prog):
+        self.opt = None
+        self.usage = None
+        self.help = None
+        self.version = None
+        self.prog = prog


It would be cleaner if the standard attributes were passed in in the initializer, preferably with the name first (I'd argue for a plugin "name" vs "opt" if we're going with -P name=val. Drop the help attribute for now, I'm wondering if we can get that from the class.__doc__ in the future, so let's leave it as a future enhancement.

def __init__(self, name, prog, usage, version=None):

Then plugins initialize themselves with super().__init__("foo", prog, usage="Enable foo")

grondo · 2025-03-07T01:34:48Z

src/bindings/python/flux/cli/plugin.py

+    def get_plugin_name(self):
+        """Return the KEY to invoke the plugin"""
+        return self.opt
+
+    def get_help_message(self):
+        """Return the message to print with --help"""
+        return self.help
+


since name and usage will be required attributes of the class, I'd drop these getters. Callers can just use plugin.name and plugin.usage

grondo · 2025-03-07T01:36:06Z

src/bindings/python/flux/cli/plugin.py

+
+
+class CLIPluginRegistry:
+    """Flux CLI plugin registry helper class"""


Note to drop "helper" here when this docstring gets expanded. This isn't just a helper class but is the plugin registry class.

grondo · 2025-03-07T02:32:32Z

src/bindings/python/flux/cli/plugin.py

+            "--plugin",
+            type=CLIPluginValue,
+            action="append",
+            help=f"{self.get_plugin_help_messages()}",


As you've noticed, this isn't going to display well in --help output.

Let's add a function to organize all the current plugin names and short usage messages into a nice table, then emit that in a custom section of the help output for the commands, e.g.:

Options provided by plugins: foo=VAL Set option foo to VAL. bar[=VAL] Set a bar with optional VAL.

Also, the add_argument() should not go in this class if we're treating them this way. Instead it just be with all the other add_argument() calls in the cli base.py.

grondo · 2025-03-07T02:33:36Z

src/bindings/python/flux/cli/plugin.py

+
+    def get_plugin_help_messages(self):
+        """Return a string that has all self.plugin help messages"""
+        ## TODO: Utilize the HelpFormatter class in argparse to make
+        ##  these messages better
+        help = ""
+        for plugin in self.plugins[1:]:
+            help += str(plugin.get_plugin_name())
+            help += str(plugin.get_help_message())
+            help += "\n"
+        return help


This should be replaced with a function to pretty print the option+usage table as noted above.

grondo · 2025-03-07T02:35:40Z

src/bindings/python/flux/cli/base.py

        self.parser = self.create_parser(prog, usage, description, exclude_io)
+        self.group = self.parser.add_argument_group("Options provided by plugins")
+        self.plugins.add_plugin_option(self.group)


Adding the -P, --plugin option should be pulled out of the function and moved to where all the other options are added.

grondo · 2025-03-07T02:41:33Z

src/bindings/python/flux/cli/base.py

+        plugins = {}
+        if args.plugin:
+            for provided_arg in args.plugin:
+                key, val = provided_arg.get_value()
+                if key in self.plugins.get_plugin_options():
+                    plugins[key] = val
+                else:
+                    raise ValueError(
+                        f"Unsupported option provided to -P/--plugin: {key}"
+                    )


It might be better if this code was part of the CLIPluginRegistry class, which should take args and extract out the active plugins, raising an exception if there was any invalid input or a plugin raised an error in preinit.

grondo · 2025-03-07T02:51:23Z

src/bindings/python/flux/cli/plugin.py

+            values (:obj:`Dict`): Dictionary of KEY[=VALUE] pairs returned
+                by parsing the arguments provided to the -P/--plugin
+                option. KEY is None if plugin was not invoked. VALUE is the
+                empty string if the plugin was invoked without VALUE.


KEY is None if plugin was not invoked.

If values is a dictionary then by "KEY is None" here do you mean "is not set in the dictionary"?

Is values the dictionary of all NAME=VALUE pairs provided to --plugin? Maybe it would be better to use a Namespace here and keep the same semantics as argparse, i.e. args.name is None if the plugin was not invoked, True if it was invoked without an arg, and a string argument that the plugin is responsible for parsing if an argument was given.

grondo · 2025-03-07T14:59:15Z

t/t2717-python-cli-plugins.t

+
+test_under_flux 4 job
+
+export FLUX_CLI_PLUGINPATH=${FLUX_BUILD_DIR}/t/cli-plugins/cli/plugins


This line needs to be moved above test_under_flux or else the job validator will not have FLUX_CLI_PLUGINPATH in its environment.

However, after fixing that every test is failing because the CustomFluxionPolicyPlugin always raises the error hi hobbs

grondo · 2025-03-07T15:24:23Z

Thanks for this work @wihobbs!

Some high level comments on the approach so far:

Perhaps it was a misstep for my prototype to allow multiple plugins per file. It would make it easier for the loader to just find and load CLIPlugin from each file found in the plugin path, instead of trying to determine which names are CLIPlugins, but avoiding the CLIPlugin base class. (This is what is done in the Validator and URIResolver plugins). The drawback is a lot of files if a site added a lot of plugins. What do you think?
Thinking ahead a bit, we may want to have plugins that are validate-only, e.g. they validate something site dependent in the jobspec. In this case we should not require a name to support -P NAME=VAL on the cli command line. Therefore, despite what I said about the plugin initializer above, maybe registration of a plugin option name, value, and usage string should be a separate function, e..g
```
class CustomPlugin(CLIPlugin):
   def __init__(self, prog):
       if prog in ("submit", "run"):
           self.add_option("foo", help="Enable foo")
```
This could also allow one plugin to supply multiple options, which could be useful.

The base class add_option method would be responsible for maintaining the set of options "provide by plugins"

wihobbs · 2025-03-07T18:45:46Z

Thanks for taking the time to dissect all of this @grondo! It was getting difficult in the thick of this project to figure out what exactly to do next, so thanks for getting me going. I'll note a few things:

I don't think having a lot of files is necessarily bad, from a center perspective this is basically how we do prolog and epilog scripts anyway, even though we could in theory have one giant one or consolidate some together
Allowing validate-only plugins is certainly a value-add, but I wonder if we need another class for this, or if we can just make all of the plugin options except prog named (optional) class attributes. I might try this today and see how it works out. This, however, wouldn't allow one plugin to supply multiple options, which might be useful.
Adding -P/--plugin to the parser where all the other options are is what I initially tried, but the method where this happens is an abstract class, so it doesn't have access to self, so self.get_help_messages doesn't work. This probably isn't a concern anymore given the proposed change to the help message.
Thanks for fixing the sharness test :)

grondo and others added 5 commits March 6, 2025 17:11

python: add prototype for submission cli plugins

e5bffca

Problem: There's no way to extend job submission cli commands in Flux via plugins. Add a prototype CLIPlugin class and CLIPluginRegistry (which loads CLIPlugins).

python: extend submission cli commands with plugins

9c4fd31

Problem: Flux submission cli commands don't support plugins. Load any plugins in the default plugin path "{confdir}/cli/plugins/*.py" and call any plugin `add_options()` and `modify_jobspec()` callbacks at appropriate points.

python: use plugins if available in the jobspec validator

497cf85

Problem: Job submission cli plugins offer a validate() callback, but this callback is not used. Load plugins and run all validate callbacks in the jobspec validator plugin.

t: test CLI plugins with example plugins

90fd106

Problem: the CLI plugin interface has no testing. Add some.

github-advanced-security bot found potential problems Mar 7, 2025

View reviewed changes

src/bindings/python/flux/cli/base.py

@@ -36,6 +36,7 @@

import flux

from flux import debugged, job, util

from flux.cli.plugin import CLIPluginRegistry, CLIPluginValue

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'CLIPluginValue' is not used.

grondo reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]: Support for command-line plugins #6691

[WIP]: Support for command-line plugins #6691

wihobbs commented Mar 7, 2025

grondo left a comment

grondo Mar 7, 2025

grondo Mar 7, 2025 •

edited

Loading

grondo Mar 7, 2025

grondo Mar 7, 2025

grondo Mar 7, 2025

grondo Mar 7, 2025

grondo Mar 7, 2025

grondo Mar 7, 2025

grondo Mar 7, 2025

grondo commented Mar 7, 2025

wihobbs commented Mar 7, 2025



		class CLIPluginRegistry:
		"""Flux CLI plugin registry helper class"""


		test_under_flux 4 job

		export FLUX_CLI_PLUGINPATH=${FLUX_BUILD_DIR}/t/cli-plugins/cli/plugins

[WIP]: Support for command-line plugins #6691

Are you sure you want to change the base?

[WIP]: Support for command-line plugins #6691

Conversation

wihobbs commented Mar 7, 2025

grondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grondo Mar 7, 2025 • edited Loading

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

grondo commented Mar 7, 2025

wihobbs commented Mar 7, 2025

grondo Mar 7, 2025 •

edited

Loading