DM-25627 make qgraph and run subcommands for pipetask

[n8pease]

No description provided.

[andy-slac]

Looks OK, check few comments. It feels like we are overdoing it with a module per command line option. There is so much boilerplate and duplication everywhere it's hard to read it.

[andy-slac]

It's rather long list, you could do from .. import opt and then @opt.something below.

[andy-slac]

f is not needed.

[andy-slac]

This does not look right, it should be a list of collections.

[andy-slac]

Is list() better than []?

[n8pease]

just a general preference for immutable objects

[timj]

If you want immutable shouldn't it be a tuple?

[n8pease]

hah. Yes.

Is list() better than []

Trying again: I don't trust that passing default=[] won't end up sharing a list instance among multiple instances of the Option, and depending on how downstream code uses the default value chaos may ensue. list() is guaranteed to create a new list instance each time it is used and is safer. OTOH, () or tuple() would both be fine since () is immutable. Probably () is slightly better because as I understand it tuple() results in an extra line of code being executed (return () or similar)

[n8pease]

and, click won't take a tuple as the default value of an option, so I'm going to stick with list()

[andy-slac]

My question was mostly rhetorical, there is no real difference between list() and [], both of them make a new empty list (but latter is shorter). What you pass as a parameter is not going to be shared, because it's a new list on every call. The sharing argument is only valid for the default argument value in the declaration of a method, there indeed you should not use mutable values as defaults.

[andy-slac]

extended-run - extend_run

[andy-slac]

I think types.SimpleNamespace class would work better for this: https://docs.python.org/3/library/types.html#types.SimpleNamespace

[n8pease]

from the docs:

SimpleNamespace may be useful as a replacement for class NS: pass

I had basically done that (class NS: pass) for the build subcommand, @timj did not like it and that's why I wrote the Args classes.

[timj]

What didn't I like?

[n8pease]

in #58, search for "cute" (I don't see a way to link to the particular conversation)

[n8pease]

since the changes are outdated (gone AFAIK), Args was declared and used like so:

class Args
    pass
args = Args()
args.qgraph = qgraph
args.save_qgraph = save_qgraph
<etc>

[timj]

I think you are over interpreting my comment. My final quote was:

At the very least a comment like you just made in the class description for Args would be useful.

(I haven't looked at the current context -- SimpleNamespace does look useful as an explicit declaration of intent)

[andy-slac]

Same comments as for qgraph.py

[n8pease]

@andy-slac

Looks OK, check few comments. It feels like we are overdoing it with a module per command line option. There is so much boilerplate and duplication everywhere it's hard to read it.

It's a fair point. I was hoping to promote reuse by just defining a shared option for each. It may err on the side of too much boilerplate. I might be able to make the shared option definition more concise, I'll think on that.

[n8pease]

Thinking about ways I could reduce the size of the shared option declarations I've realized I can use functools.partial to bind option decorator arguments to reduce almost all the boilerplate that was needed for MWOptionDecorator subclasses, and can instead have MWOptionDecorator instances be the option decorators that are used. (@andy-slac this is very similar but slightly more than what I presented to you in Slack). MWOptionDecorator becomes like this

class MWOptionDecorator:

    def __init__(self, *param_decls, **kwargs):
        self.partialOpt = partial(click.option, *param_decls, cls=MWOption, **kwargs)
        opt = click.Option(param_decls, **kwargs)
        self._name = opt.name
        self._opts = opt.opts

    def name(self):
        return self._name

    def opts(self):
        return self._opts

    def __call__(self, *args, **kwargs):
        return self.partialOpt(*args, **kwargs)

(initing the Option and fetching its name and opts flags is currently the best way to get these vars from the Option. I've sent a question to the click devs about making the logic for this available by static methods or similar.)

Its use to define shared options is very terse, for example:

butler_config_option = MWOptionDecorator("-b", "--butler-config",
                                         help="Location of the gen3 butler/registry config file.",
                                         type = MWPath(dir_okay=False, file_okay=True, readable=True))

data_query_option = MWOptionDecorator("-d", "--data-query",
                                      help="User data selection expression."
                                      metavar="QUERY")

I think this will be a big improvement.