@johanneskoester an update!

I completely agree with your sentiment - I was not happy with the args design either, and I think using a dataclass is a great strategy. Specifically what I decided to try for this first go is:

1. Allowing the custom plugin to define a vanilla dataclasses.dataclass, and with arguments that have a prefix specific to the plugin (to namespace them in the argument space)
2. Using a function from https://github.com/mivade/argparse_dataclass that can add the dataclass to the parser (and it's a simple conversion of a datatype to an arguiment). This is why the namespacing is important, otherwise you'd get a conflicting name error)
3. The args (with user choices for the plugin too!) are then converted back to the dataclass, which is passed forward to the custom executor plugin!

Note that the above approach doesn't require an extra dependency in snakemake, but just in the plugin, which I like. I also provide the "parse your args back into the dataclass" as a snakemake.plugins function, which (I hope) along with documentation / examples will encourage folks to honor their plugin's namespace. We only (by default) return custom dataclass arguments that also adhere to the executor plugin namespace. Anyone that wants to customize that can proceed at their own risk! And note that outside of the dataclass, any "global" snakemake arg that is used by a plugin would be provided as it is now - passed from args into some other argument to the executor. We could eventually clean this up and have some way for multiple plugins to add the same argument, but that's probably out of scope for this early work.

For the executor specific ones, we should define a prefix that comes from the plugin name (e.g. just the name of the executor). For the others, the name is kind of free to choose. In order to let the executor access all those general args, it makes probably sense to put everything that is currently passed as individual parameters to the snakemake function into a dataclass as well. Then, the executor plugin can just get two dataclass objects (the global args and the executor specific args).

Oh that's a fantastic idea - I could follow up with this PR, unless you think it's absolutely essential here (but would be a lot of moving pieces for one PR).

And I'm not sure where those other commits came from - I hope I didn't bork the git history. I can try a rebase, although now I'm nervous to screw the entire thing up. Perhaps when it is definitely done I will just redo the whole thing over freshly.

I think the plugin implementation should require a plugin to provide the minimum compatible Snakemake version. A natural value for the beginning would be the first release with plugin functionality. But later, some plugins might require more recent versions of Snakemake and the user should get a proper error message instead of just some cryptic exceptions.

I didn't add this yet, but I will after posting this!

And finally: yes, every executor should eventually be migrated into a plugin.

Agree! This init file and passing forward arguments has seriously been giving me nightmares for years... I'm glad we have a plan to clean things up a bit!

For details on the current state of the plugin setup, we have an early writeup here: https://github.com/snakemake/snakemake-executor-flux#how-does-it-work. I'll update it soon for the snakemake version (and will validate it's defined).

I think the other commits are just coming from merging in upstream. That should be fine.

In order to get the plugin interface right, it seems to me that the dataclass approach for the other arguments should be done first. But indeed that can happen in a separate PR. It would be awesome if you really would like to do that!
Then, the plugins would be also simpler because I guess the add_args and parse functionality can then be moved into Snakemake itself.

Then, the plugins would be also simpler because I guess the add_args and parse functionality can then be moved into Snakemake itself.

Most of the functionality within those functions can be provided by snakemake, but I expose them on the level of the plugin to give the plugins control to dictate logic. But actually, we could remove them from validation and then do a check (and call the default provided in snakemake) if they aren't present!

What did you have in mind for porting current executors to plugins? Having the same structure but adding them to snakemake dependencies so they are always installed?

Just rebased - I'm not sure why the local test is reporting there is no module packaging, because it's definitely in test-environment.yaml!

@johanneskoester it is reporting that packaging isn't installed, however I've added it to the snakemake install requires, the test step, and the container. I'm not familiar with this setup - can you take a look and advise where it needs to be added?

__real_file__ = __file__; __file__ = '/tmp/snakemake-lmz9cooy/script.py';
  File "/mnt/snakemake/snakemake/__init__.py", line 23, in <module>
    from snakemake.workflow import Workflow
  File "/mnt/snakemake/snakemake/workflow.py", line 28, in <module>
    from snakemake.scheduler import JobScheduler
  File "/mnt/snakemake/snakemake/scheduler.py", line 27, in <module>
    from snakemake.plugins import executor_plugins
  File "/mnt/snakemake/snakemake/plugins.py", line 10, in <module>
    import packaging.version
ModuleNotFoundError: No module named 'packaging'

And I'd comment if this is going to be a pain point, we should fall back to being more lenient to explicit version checking. Maybe there is a simpler way, to try importing what we need for the specific plugin and then reporting something is missing/wrong.

What did you have in mind for porting current executors to plugins? Having the same structure but adding them to snakemake dependencies so they are always installed?

Yes, exactly that.

Most of the functionality within those functions can be provided by snakemake, but I expose them on the level of the plugin to give the plugins control to dictate logic. But actually, we could remove them from validation and then do a check (and call the default provided in snakemake) if they aren't present!

Actually, I would prefer if plugins would not access the argparser themselves at all. Otherwise, it will become very hard to change it in any way. Further, since Snakemake can be used as an API, there will be cases where one wants to use a plugin without even having an argparser (you just directly call the snakemake function in that case).

@johanneskoester how would we get custom arguments then?

The executor provides information about its required input via the dataclass that it defines. Like this:

from dataclasses import dataclass, field

@dataclass
class ExecutorParameters:
    machine: str = field(default="standard-e2", metadata={"help": "Machine type to use for Google Batch execution."})

For the two use cases:

• CLI: we add the fields to the argument parser of Snakemake, and prefix every field with the name of the executor (e.g. in the above case we generate --google-batch-machine if the executor plugin package is called snakemake-executor-google-batch).
• API: an instance of the dataclass is simply created by the developer who uses the API and passed to the snakemake function.

I was wondering whether we should have a way to specify and access non executor specific arguments, but I think this is not necessary. At least not beyond those parameters that are already passed to the current exectuors by the scheduler. This also means that we don't have to modify the entire Snakemake towards dataclass usage for now. We can simply start with the executor plugins.

CLI: we add the fields to the argument parser of Snakemake, and prefix every field with the name of the executor (e.g. in the above case we generate --google-batch-machine if the executor plugin package is called snakemake-executor-google-batch).

So you are OK with the plugin dynamically adding arguments (meaning snakemake adding them based on the dataclass, the opposite direction that we have now), you just don't want to explicitly hand over args? What about cases where we need further customization of the args for the plugin to work?

That could work, although if more than one plugin is registered, we can't enforce any of the arguments to be required, and if we can't pass the args forward to validate, it will be more of a "try to run and hope it works."

Let me know if that is what you have in mind and I can make changes.

Taking a look again - the case for just having the arguments from the dataclass added, as is, is already easy - we can just tweak this function:

def add_args(parser):
    """
    Allow the custom executor to modify the parser as needed.

    Note that we do not here, but it's recommended to use your custom
    parser GROUP_IDENTIFIER as a prefix to honor the namespace.
    """
    _add_dataclass_options(ExecutorParameters, parser)

To add the plugin prefix to the ExecutorParameters before we provide to that function. But what about needing to tweak logic within snakemake apriori? We have a function for that too:

plugins.args_to_dataclass(args, ExecutorParameters)

But I'm thinking about cases in the current init where we have big if statements like "if this executor, change the filesystem setting to X." I'm guessing your thinking is that we will eventually have some kind of shared executor parameters that somehow handle that? Should we not consider that for now, and just move these existing functions to (instead of being called by the plugin) to be called by snakemake directly with the context of the plugin?

To add the plugin prefix to the ExecutorParameters before we provide to that function. But what about needing to tweak logic within snakemake apriori? We have a function for that too:

plugins.args_to_dataclass(args, ExecutorParameters)

But I'm thinking about cases in the current init where we have big if statements like "if this executor, change the filesystem setting to X." I'm guessing your thinking is that we will eventually have some kind of shared executor parameters that somehow handle that? Should we not consider that for now, and just move these existing functions to (instead of being called by the plugin) to be called by snakemake directly with the context of the plugin?

I would handle that by requiring that the plugin has a dict called snakemake_behavior.
In Snakemake, this dict can then be queried for certain things (always with defaults so that things added in future versions of Snakemake are in any case backwards compatible and so that the plugin can choose to just use the default for certain things), e.g.

executor_plugin.snakemake_behavior.get("use_threads", False)
executor_plugin.snakemake_behavior.get("assume_shared_fs", True)
executor_plugin.snakemake_behavior.get("default_remote_provider", None)
executor_plugin.snakemake_behavior.get("default_remote_prefix", None)

This queries should happen inside of the snakemake function (of course by calling a helper), because they have to happen both in the API and in the CLI case.

@johanneskoester ignoring the black failure for now (I somehow have the same version 23.3.0 installed but it is saying there are no changes so we can look at that detail later) I've refactored the plugin structure so that the plugin itself does no additional parsing of either the parser, or the args. The plugin now can define whatever fields they like, eg..,

@dataclass
class ExecutorParameters:
    help: Optional[str] = None
    description: Optional[str] = None

And then snakemake iterates through each plugin's fields, and adds them with the plugin prefix. E.g., the above becomes:

--flux-help
--flux-description

And then to go the other way around, we take that dataclass, remove the prefix, and parse back into the original. An edge case I'm thinking abiout

More detail updated here:

https://github.com/snakemake/snakemake-executor-flux. I think this is probably closer to what you had in mind - let me know what we should work on next (and what might be scoped to this PR vs done in a follow up).

I was wondering if you would like to migrate your plugin code from here into the missing pieces of the new interface via a PR on that repository.

What do you think?

If that's the direction you like, it works for me! One suggestion - should we have a different prefix name for this library so it doesn't detect itself as a plugin? And also - if you have vision for other components of snakemake to be plugins, would you want to manage another interface (or go off of this one?)

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
15 Code Smells

No Coverage information
0.0% Duplication

Woohoo!! What would you like to do next @johanneskoester ? I can start refactoring some the current executors into plugins, or since we already have one for flux, remove it here. Let me know your preferences!