DM-25627 make qgraph and run subcommands for pipetask #347

n8pease · 2020-08-11T03:29:55Z

No description provided.

timj · 2020-08-11T22:56:59Z

python/lsst/daf/butler/cli/utils.py

+                            cls=OptionSection)(f)
+
+
+class MWPath(click.Path):


When specifying a butler configuration file you can't use anything that assumes that it can open a local file. From the ctrl_mpexec PR I see that this is used for butler configs. Butler configs can be URIs that may or may not include the "butler.yaml" at the end (they can be s3:// etc). You could use ButlerURI.exists() to see if it exists but better to pass the string to Butler() directly without trying to check it.

andy-slac

Looks OK in general, bunch of minor comments. My concern after reading through all new additions is that it looks like you have to fight click to force it to do what we want. To my taste it's too much code that we did not have to write.

python/lsst/daf/butler/cli/butler.py

andy-slac · 2020-08-11T20:54:21Z

python/lsst/daf/butler/cli/opt/collection_type.py

@@ -22,11 +22,11 @@

 import click

-from lsst.daf.butler.cli.utils import MWOption
+from lsst.daf.butler.cli.utils import MWOption, MWOptionDecorator


Could use relative import for this?

andy-slac · 2020-08-11T21:13:34Z

python/lsst/daf/butler/cli/utils.py

+    @classmethod
+    def _update(cls):
+        """Use the click.Option to generate the kwarg name and the list of
+        flags


Here and few other places - need period at the end of sentences.

andy-slac · 2020-08-11T21:18:41Z

python/lsst/daf/butler/cli/utils.py

+        defaultHelp : `str`
+            The default help text.
+        """
+        pass


This should probably be raise NotImplementedError() just in case someone calls this method, same for optionFlags.

andy-slac · 2020-08-11T21:28:19Z

python/lsst/daf/butler/cli/utils.py

    separator : str, optional
        The character that separates key-value pairs. May not be a comma or an
        empty space (for space separators use Click's default implementation
        for tuples; `type=(str, str)`). By default "=".
    unseparated_okay : `bool`, optional
        If True, allow values that do not have a separator. They will be
        returned in the values dict as a tuple of values in the key '', that
-        is: `values[''] = (unseparated_values, )`.
+        is: `values[''] = (unseparated_values, )`. By default False.
+    return_type : `str`, one of "dict" or "tuple"


Instead of string you can accept type itself (dict or tuple), would at least reduce chance of typo (and need to check for that).

andy-slac · 2020-08-11T22:13:10Z

python/lsst/daf/butler/cli/utils.py

+
+    Parameters
+    ----------
+    optionDecorators : `list` or `tuple` [`MWOptionDecorator`]


Should it be mwOptions instead of optionDecorators?

andy-slac · 2020-08-11T22:14:06Z

python/lsst/daf/butler/cli/utils.py

+    """
+
+    def __init__(self, mwOptions=None):
+        self.functionKwargs = {}


I'm confused, mwOptions is not used at all?

andy-slac · 2020-08-11T22:18:16Z

python/lsst/daf/butler/cli/utils.py

+
+        Parameters
+        ----------
+        mwOptions : `list` or `tuple` [`MWOption`]


Can be iterable instead of list or tuple?

well I guess it could be a dict but it really oughtn't be.

andy-slac · 2020-08-11T22:30:50Z

python/lsst/daf/butler/tests/cliOptionTestBase.py

+            self.run_test(helper.cli,
+                          self.makeInputs(self.optionFlag, val),
+                          self.verifyError,
+                          "")  # the error strings are too varied, don't test.f


"test.f"? You did not finish sentence 😸

andy-slac · 2020-08-11T22:53:28Z

tests/test_cliUtils.py

+    if False:
+        ForwardObjectsTest.cli()
+    else:
+        unittest.main()


Why if False?

was temp. will remove.

n8pease · 2020-08-13T22:10:49Z

@andy-slac

Looks OK in general, bunch of minor comments. My concern after reading through all new additions is that it looks like you have to fight click to force it to do what we want. To my taste it's too much code that we did not have to write.

Why does it feel that way to you? I didn't feel like I was fighting or forcing click, rather extending it, where I wanted to slightly change behavior or get a bit of information.

I think I could remove a lot of the code that "extends" Click by getting rid of chained commands and command forwarding and going back to your definitions of build, qgraph, and run. Maybe that's a better way to go?

andy-slac · 2020-08-14T17:58:02Z

My impression that click is not cooperating is mostly from looking at utils.py and what you had to do for pipeline actions in ctrl_mpexec. option_section looks more like a hack in click than an extension. All extra classes that you added to support chaining/forwarding also tells me that click idea of how things should work is different from what we need.

And I'm not a fan of chaining, so if you say you want to drop it with all extra classes I won't object 🙂

Another idea that occurred to me - the list of options is big and probably growing, if we want to simplify reuse it may be worth to "package" related options into one decorator, i.e. instead of:

@output_option()
@output_run_option()
@extend_run_option()
@replace_run_option()
@prune_replaced_option()
def something()

do

@all_output_options()
def something()

(that example is for ctrl_mpexec options but we may have something similar in other places).

n8pease · 2020-08-14T23:26:31Z

That all makes sense. I had coded up a batch decorator for the butler config options, but had hoped chaining would prevent the need to use it. Of course it turns out that qgraph and run still accept the butler options anyway. Given that , and that the chaining implementation requires more hacking on Click than you'd like, and that the number of lines of code in this review is more than people would like, it's sounding to me like the way to go is to take out chaining and making the commands behave the same as with the existing ctrl_mpexec command line parser.

timj · 2020-08-14T23:32:23Z

I'm not completely averse to chaining, but it's impossible to work out what chaining really means from this pull request. I thought we had agreed to a community post to discuss things.

n8pease · 2020-08-15T01:09:57Z

A community post after getting code checked in, which assumed I could get it past review 🙃

timj · 2020-08-17T16:05:17Z

Ok. Firstly, please fix the tests so that the PR can build.

Secondly, we have a chicken and egg problem. It's really hard to review the pipetask code without the context of understanding subcommands. It's a very large pull request to understand without that context. Since pipetask2 is not something that people are using it seems like we can only break this impasse by merging the ctrl_mpexec PR, writing the community post, letting people experiment, and then being prepared to rip the code out again.

n8pease · 2020-08-18T04:07:10Z

Ok, sounds fine. I just started a Jenkins build, once that's complete I'll check it in. I will start working on the community post tomorrow morning.

Allows section headings to be injected between options when the Click command help is printed.

click.Path can verify a file (or dir) does exist, but does not have support for not-exists. MWPath subclasses Path and provides that feature.

Called MWOptionDecorator. This facilitates unit testing by declaring abstract methods and helper methods that allow the unit tests to determine things about the option, like what the parameter declarations (option flags, command function keyword argument name) will be. This change also changes all abstract class properties in shared options to abstract static methods.

Even though arguments don't use a common abstract base class (yet...) they need to follow the change made to shared options, to use a static method for 'defualtHelp' instead of a class property, because the api needs to match. There's probably room for an MWArgumentDecorator class here that would be very similar to MWOptionDecorator. They could even have a common MWParameterDecorator base. TBD when it's just insanity. For now it seems Good Enough to have the option decorator, and just make this change; arguments are infrequent enough.

It was making a directory with the temporary file name. This fix splits the tempfile name and calls makedirs when needed, and then creates the file in that location.

In some cases now, the command test calls multiple subcommands in a single execution (e.g. "pipetask2 qgraph <args> run <args" calls the "qgraph" and "run" subcommands). This change allows the test code to assert both mocked commands were called with the expected arguments.

Now if there is a blank line between paragraphs in a triple-quoted string, unwrap will preserve that blank line.

split_kv used to put all values into a dict. Some pipetask options want the value to be a tuple of two-item tuples. Some pipetasks reverse the key and value so that --opt k=v becomes dict(v=k) or ((v, k),). This adds those options to the split_kv callback, they can be set when passing the callback by using functools.partial.

Option forwarding allows CLI option values passed to one subcommand to be "forwarded" to the next subcommans without the user having to re-enter the same options for the next subcommands. For example, "pieptask qgraph <butler options> run" allows butler options passed to the qgraph subcommand to be used by the run subcommand.

Instead of print_exc. This eliminates the need for a temporary string object.

MWOptionDecorator used to be a subclass for shared option decorator types. This changes it to be a factory that generates a callable type that returns a click.option with the shared parameters prepopulated (using functools.partial). It also implements a few helper functions for e.g. getting the name of the argument with which the option will call the command function.

timj reviewed Aug 11, 2020

View reviewed changes

andy-slac approved these changes Aug 11, 2020

View reviewed changes

n8pease force-pushed the tickets/DM-25627 branch 2 times, most recently from 864854d to 14296fe Compare August 18, 2020 03:57

n8pease added 16 commits August 18, 2020 09:09

remove incorrect docstring

3c4ac65

add section separator for CLI help options

3f60071

Allows section headings to be injected between options when the Click command help is printed.

add MWPath to support file non-exist checking

da1a01a

click.Path can verify a file (or dir) does exist, but does not have support for not-exists. MWPath subclasses Path and provides that feature.

add CLI option tests

0cd7013

debug a traceback on import error

7628c6a

change abstract properties to abstract methods

bc175ee

add api to reset Mocker

b527745

fix tempfile generation in CLI command tests

1dc3d54

It was making a directory with the temporary file name. This fix splits the tempfile name and calls makedirs when needed, and then creates the file in that location.

allow section breaks in unwrap util

da441e7

Now if there is a blank line between paragraphs in a triple-quoted string, unwrap will preserve that blank line.

use traceback.format_tb

15190ac

Instead of print_exc. This eliminates the need for a temporary string object.

n8pease force-pushed the tickets/DM-25627 branch from 14296fe to 143d80c Compare August 18, 2020 16:10

n8pease merged commit a2728ea into master Aug 18, 2020

timj deleted the tickets/DM-25627 branch February 16, 2024 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-25627 make qgraph and run subcommands for pipetask #347

DM-25627 make qgraph and run subcommands for pipetask #347

n8pease commented Aug 11, 2020

timj Aug 11, 2020

andy-slac left a comment

andy-slac Aug 11, 2020

andy-slac Aug 11, 2020

andy-slac Aug 11, 2020

andy-slac Aug 11, 2020

andy-slac Aug 11, 2020

andy-slac Aug 11, 2020

andy-slac Aug 11, 2020

n8pease Aug 13, 2020

andy-slac Aug 11, 2020

andy-slac Aug 11, 2020

n8pease Aug 12, 2020

n8pease commented Aug 13, 2020 •

edited

andy-slac commented Aug 14, 2020

n8pease commented Aug 14, 2020 •

edited

timj commented Aug 14, 2020

n8pease commented Aug 15, 2020

timj commented Aug 17, 2020

n8pease commented Aug 18, 2020

DM-25627 make qgraph and run subcommands for pipetask #347

DM-25627 make qgraph and run subcommands for pipetask #347

Conversation

n8pease commented Aug 11, 2020

Choose a reason for hiding this comment

andy-slac 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

n8pease commented Aug 13, 2020 • edited

andy-slac commented Aug 14, 2020

n8pease commented Aug 14, 2020 • edited

timj commented Aug 14, 2020

n8pease commented Aug 15, 2020

timj commented Aug 17, 2020

n8pease commented Aug 18, 2020

n8pease commented Aug 13, 2020 •

edited

n8pease commented Aug 14, 2020 •

edited