mgr_module.py: make keyword style argument passing explicit #36910

jan--f · 2020-08-31T12:09:52Z

This PR changes the recent addition to CLICommand to support kwargs style passing. With this a module author has to explicitly mark arguments that can be passed with a keyword.

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

sebastian-philipp · 2020-08-31T12:13:40Z

jenkins render docs

ceph-jenkins · 2020-08-31T12:30:54Z

Doc render available at http://docs.ceph.com/ceph-prs/36910/

sebastian-philipp · 2020-08-31T12:40:34Z

interesting. those calls are missing from https://docs.ceph.com/ceph-prs/36910/api/mon_command_api/

sebastian-philipp · 2020-08-31T12:44:31Z

what I still don't quite understand is:

fs snap-schedule status --path=<mypath> --subvol=<subvol>

already works. But kw=true is not expose to the ceph binary, how is this supposed to work?

I mean, I can still say:

fs snap-schedule status <mypath> <subvol>

jan--f · 2020-08-31T12:59:00Z

interesting. those calls are missing from https://docs.ceph.com/ceph-prs/36910/api/mon_command_api/

Looks like commands that are setup via the CLICommand decorator (or its variants) are just not extracted. E.g all dashboard dashboard ac-user-* commands are also missing.

jan--f · 2020-08-31T13:01:41Z

what I still don't quite understand is:
fs snap-schedule status --path=<mypath> --subvol=<subvol>

Unfortunaltey things can't look quite like this. The command would be

fs snap-schedule status path=<mypath> subvol=<subvol>

already works. But kw=true is not expose to the ceph binary, how is this supposed to work?
An argument path=/foo is passed as a string.

I mean, I can still say:
fs snap-schedule status <mypath> <subvol>

Right, that will still work. I'd be up for adding a kw=always variant if one wants to force passing as kwarg, but I don't really see a use case for this.

jan--f · 2020-09-01T13:24:56Z

Hmm seems like ceph_argparse.py needs a change as well. Looking into it.

Instead of assuming a kwarg when a '=' is encountered, this commit changes the CLICommand decorator to require an explicit 'kw=true' specification in a command definition to add the option to parse it as a kwarg. Arguments marked with kw=true can either be passed kwarg style (arg_name=value) or as a positional argument. Once a kwarg style argument was seen, the following ones must not be passed as positionals. Signed-off-by: Jan Fajerski <jfajerski@suse.com>

When setting up the module commands we can warn if the developer defined a positional after a keyword argument. Signed-off-by: Jan Fajerski <jfajerski@suse.com>

Signed-off-by: Jan Fajerski <jfajerski@suse.com>

jecluis

I think the idea makes sense, especially given that the support already exists but the current implementation seems broken due to assuming any = will be handled as the separator for a key/value pair.

I do have a few comments on the approach though.

jecluis · 2020-09-01T14:36:58Z

src/pybind/mgr/mgr_module.py

@@ -299,6 +300,12 @@ def _parse_args(self):
                    arg_d[k] = v
                else:
                    self.args_dict[v] = arg_d
+                if k == 'kw' and v == 'true':


I find the kw keyword a bit too weird, especially since we're then dropping "kwarg" terms bellow. I find going with something a bit more descriptive (especially since we're using this on the CLI descriptor) would be ideal. Something like "requires_args" or "with_map_args". More to type, but I'm sure with a bit of creativity we can find a better keyword ;)

maybe k/v argument? kv_arg?

kw stands for keyword, borrowed from pythons keywords arguments. kwarg might be better and should be familiar to python developers.

but if we are exposing kw as a thing to the user (like we do on returning EINVAL later in the patch), this is no longer just for developer consumption and, I'd say, a more relatable term might be better.

Yeah fair point.

jecluis · 2020-09-01T14:49:44Z

src/pybind/mgr/mgr_module.py

+                else:
+                    if kwargs_switch:
+                        # we saw a kwarg earlier and now a positional
+                        logging.warn('Found positional argument after kwarg')


is there a reason why we'd warn about positional arguments after a k/v pair was found? I don't see a particular technical difficulty in handling them, especially since we know which arguments are expected to be k/v pairs.

Here (while setting up the command and argument structures) I only warn since the kw=true currently enables optional passing as a kwarg. The user can also pass the arguments as positional arguments.

jecluis · 2020-09-01T14:52:46Z

src/pybind/mgr/mgr_module.py

+                if not kwargs_switch and isinstance(cmd_dict[a], str) and '=' in cmd_dict[a]:
+                    # We consider this a kwarg. Set the switch so that all
+                    # future args are parsed as kwargs too.
+                    mgr.log.debug('Seen a kwarg, assuming all following args are kw style')


I asked this above, but is there a technical reason why we can't handle positional arguments after k/v pairs?

We're not even allowing spaces between the key portion, the equal sign, and the value portion, so we're not actually dealing with 3 arguments in the CLI, just the one. Why not allowing other arguments to come after?

The reasoning is that once we have seen a kwarg and it is followed by a positional, we don't know what that positional argument is (since it is identified by its position in the arg list). This is consistent with python behaviour.

I see the point, but aren't these being pre-handled by argparse so that they arrive as a dictionary as is? IIRC, and I may be conflating argparse and command parsing in the mgr module, arguments are identified by their dictionary k/v pairs rather than list positional arguments; no?

Once we're in mgr module land, yes, we have the dict. But the dict is filled by position on the command line iiuc. But I need to look closer into ceph_argparse, the kwargs parsing should probably happen there.

jecluis · 2020-09-01T15:07:35Z

Okay, crazy idea: why not implementing this as a new type for argparse, such that you can have a map type?

jan--f · 2020-09-02T14:14:44Z

Okay, crazy idea: why not implementing this as a new type for argparse, such that you can have a map type?

The argparse types are value types. We want to use those for positionals as well as kwarg style. I realise that in order to use the kwarg style they argument currently has to be a CephString, this is insufficient.
As mentioned above, I'll look into pushing the this code into ceph_argparse in order to play nicer with the types. It's also where that code should live.

stale · 2020-11-01T16:37:07Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2021-01-12T13:13:06Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

sebastian-philipp · 2021-05-31T08:49:17Z

relates to https://tracker.ceph.com/issues/50954

ceph-jenkins · 2021-05-31T09:03:35Z

Doc render available at https://ceph--36910.org.readthedocs.build/en/36910/

leseb · 2021-06-07T14:17:09Z

superseded by #41509?

jan--f added pybind mgr labels Aug 31, 2020

jan--f requested a review from sebastian-philipp August 31, 2020 12:09

sebastian-philipp requested a review from matthewoliver September 1, 2020 11:50

Jan Fajerski added 3 commits September 1, 2020 16:44

pybind/mgr/mgr_module: check consistency of kw=true arguments

2e6449c

When setting up the module commands we can warn if the developer defined a positional after a keyword argument. Signed-off-by: Jan Fajerski <jfajerski@suse.com>

pybind/mgr/snap-schedule: add kw=true flags to kw style arguments

225e6ce

Signed-off-by: Jan Fajerski <jfajerski@suse.com>

jan--f force-pushed the mgr_module-explicit-kwargs branch from 5331811 to 225e6ce Compare September 1, 2020 14:44

jecluis requested changes Sep 1, 2020

View reviewed changes

jan--f added the DNM label Sep 2, 2020

stale bot added the stale label Nov 1, 2020

github-actions bot added the needs-rebase label Jan 12, 2021

sebastian-philipp closed this May 31, 2021

sebastian-philipp reopened this May 31, 2021

stale bot removed the stale label May 31, 2021

sebastian-philipp closed this Sep 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr_module.py: make keyword style argument passing explicit #36910

mgr_module.py: make keyword style argument passing explicit #36910

jan--f commented Aug 31, 2020

sebastian-philipp commented Aug 31, 2020

ceph-jenkins commented Aug 31, 2020

sebastian-philipp commented Aug 31, 2020

sebastian-philipp commented Aug 31, 2020 •

edited

jan--f commented Aug 31, 2020

jan--f commented Aug 31, 2020

jan--f commented Sep 1, 2020

jecluis left a comment

jecluis Sep 1, 2020

jecluis Sep 1, 2020

jan--f Sep 1, 2020

jecluis Sep 1, 2020

jan--f Sep 2, 2020

jecluis Sep 1, 2020

jan--f Sep 1, 2020

jecluis Sep 1, 2020

jan--f Sep 1, 2020

jecluis Sep 1, 2020

jan--f Sep 2, 2020

jecluis commented Sep 1, 2020

jan--f commented Sep 2, 2020

stale bot commented Nov 1, 2020

github-actions bot commented Jan 12, 2021

sebastian-philipp commented May 31, 2021

ceph-jenkins commented May 31, 2021

leseb commented Jun 7, 2021

mgr_module.py: make keyword style argument passing explicit #36910

mgr_module.py: make keyword style argument passing explicit #36910

Conversation

jan--f commented Aug 31, 2020

Checklist

sebastian-philipp commented Aug 31, 2020

ceph-jenkins commented Aug 31, 2020

sebastian-philipp commented Aug 31, 2020

sebastian-philipp commented Aug 31, 2020 • edited

jan--f commented Aug 31, 2020

jan--f commented Aug 31, 2020

jan--f commented Sep 1, 2020

jecluis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jecluis commented Sep 1, 2020

jan--f commented Sep 2, 2020

stale bot commented Nov 1, 2020

github-actions bot commented Jan 12, 2021

sebastian-philipp commented May 31, 2021

ceph-jenkins commented May 31, 2021

leseb commented Jun 7, 2021

sebastian-philipp commented Aug 31, 2020 •

edited