DM-24330: add ability to run an obs_base command via the butler command #258

n8pease · 2020-04-13T23:19:36Z

No description provided.

n8pease · 2020-04-13T23:29:15Z

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

+    verbose = len(sys.argv) > 1 and "-v" == sys.argv[1]
+    logging.basicConfig(level=logging.DEBUG if verbose else logging.INFO)
+    log.setLevel(lsst.log.Log.DEBUG if verbose else lsst.log.Log.INFO)
+    return getCli()()


This file doesn't have to be called butler.py; it's called by the file at bin/butler which is the file name that causes the butler script to be named butler. This could be named e.g. butlerCli.py if that could reduce confusion between this and the file that owns the Butler class (_butler.py). I wasn't sure which way to go on that.

I don't think it really matters given namespacing. @TallJimbo ?

timj

This looks good. I do have a few comments though.

timj · 2020-04-13T23:52:07Z

doc/lsst.daf.butler/index.rst

-   :prog: validateButlerConfiguration.py
-   :groups:
-
+.. click:: lsst.daf.butler.script.butler:cli


script.butler or cli.butler?

timj · 2020-04-13T23:53:59Z

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

+def _getPluginList():
+    pluginModules = os.environ.get("DAF_BUTLER_PLUGINS")
+    if pluginModules is None:
+        return None


Shouldn't this return an empty list? Otherwise _importPlugins is going to get confused.

timj · 2020-04-13T23:58:21Z

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

+
+
+@click.group()
+# this group doesn't use the verbose command but does allow (ingore) it,


Typo: ingore

timj · 2020-04-14T00:00:13Z

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

+    verbose = len(sys.argv) > 1 and "-v" == sys.argv[1]
+    logging.basicConfig(level=logging.DEBUG if verbose else logging.INFO)
+    log.setLevel(lsst.log.Log.DEBUG if verbose else lsst.log.Log.INFO)
+    return getCli()()


I don't think it really matters given namespacing. @TallJimbo ?

timj · 2020-04-14T00:01:06Z

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

+
+
+def main():
+    verbose = len(sys.argv) > 1 and "-v" == sys.argv[1]


I thought click meant that sys.argv wasn't involved. Are we causing trouble by wanting every command to have a --verbose flag?

there's a couple things here:

I wanted to be able to log the loading of plugins, this is before the invocation of the top-level Click object.

As far as I can tell, logging does not like to be set up more than once. It seems there are some ways to force it but I couldn't get consistent results. (in Python 3.8 logging.basicConfig adds an arg force which may help with this, but I think we are on 3.7?)

The best solution I could come up with was to have the verbose option handled outside of Click, and have it turned on or off globally. Not every command will have a --verbose flag. The usage is

Usage: butler [OPTIONS] COMMAND [ARGS]... Options: -v, --verbose Turn on debug reporting. --help Show this message and exit. Commands: create Create an empty Gen3 Butler repository. dump-config Dump either a subset or full Butler configuration to... validate-config Validate the configuration files for a Gen3 Butler...

I'm fine with every command having this. If we only want one special option though I would be happy to abandon --verbose and replace it with --log-level and have it default to WARN. This gives people a lot more control over DEBUG and INFO.

ok, I'll change --verbose to --log-level

timj · 2020-04-14T00:17:56Z

tests/test_cliOptionRepo.py

+
+
+@click.command()
+@repo_option()  # required defaul val is False


typo: defaul

timj · 2020-04-14T03:16:08Z

tests/test_commandLine.py

+            result = runner.invoke(butler.getCli(), ["dump-config", "here"])
+            self.assertEqual(result.exit_code, 0)
+            # check for some expected keywords:
+            self.assertTrue("composites" in result.stdout)


Hmm. Can't we read the output as YAML, parse that, and then check for composites in the dict?

Also, please use assertIn.

timj · 2020-04-14T03:21:02Z

ups/daf_butler.table

@@ -11,3 +11,4 @@ setupRequired(afw)

 envPrepend(PATH, ${PRODUCT_DIR}/bin)
 envPrepend(PYTHONPATH, ${PRODUCT_DIR}/python)
+envPrepend(DAF_BUTLER_PLUGINS, lsst.daf.butler.cli.cmd)


I don't think we want this for butler itself because the butler already knows where its own scripts are. The butler standard set should always work regardless.

For testing I'd create a trivial command line plugin that echoes back the subcommand name. In the test set the environment variable to include the path to that test code and run the new subcommand and check the output.
Note that you can create a class inline in a test and use getFullTypeName on that class and doImport will work on it (since Python will realize it has already been imported). -- the trick is triggering the read of DAF_BUTLER_PLUGINS if some other test has already triggered it.

timj · 2020-04-14T03:24:06Z

tests/test_commandLine.py

+            self.assertTrue("datastore" in result.stdout)
+            self.assertTrue("storageClasses" in result.stdout)
+
+    def testDumpConfig_file(self):


Can you check that the subsetting command line option works as well please?

timj · 2020-04-14T03:25:22Z

tests/test_commandLine.py

+    def testValidateConfig(self):
+        """Test validating a valid config."""
+        runner = click.testing.CliRunner()
+        with runner.isolated_filesystem():


Also check that an unknown dataset type causes a bad exit status.

timj · 2020-04-16T21:52:53Z

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

+
+def _initLogging(logLevel):
+    log = lsst.log.Log.getLogger("lsst.daf.butler")
+    if "critical" == logLevel:


I thought I made a comment on this but I can't find it any more. Just in case it got lost: please don't do this with if statements. The logging module has methods for translating the text string to a logging level and lsst.log has method for converting a logging level to an lsst.log level. See astro_metadata_translator command line tool for example code.

timj

Don’t use absolute imports when you are wanting relative imports.

timj · 2020-04-17T18:13:21Z

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

+import logging
+import os
+
+from lsst.daf.butler.cli import cmd as butlerCommands


Please use relative imports in all this code

timj

Minor comments. Some really impressive test code.

timj · 2020-04-20T23:39:54Z

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

+
+
+def funcNameToCmdName(functionName):
+    """Change underscores, used in fucntions, to dashes, used in commands."""


Typo: fucntions

timj · 2020-04-20T23:42:21Z

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

+        try:
+            return doImport(pluginName)
+        except (TypeError, ModuleNotFoundError, ImportError) as err:
+            logging.warning("Could not import plugin from %s, skipping.", pluginName)


Does this add the message to the right logger? I think we want

log = logging.getLogger(__name__)

at the top and then 'log.warning(). I know that this logger is only going to be triggered from the command line tooling but we should be consistent about log hierarchies (unless you are going to tell me that it's required to use the default logger).

nope, I'm just confused about logging. Will fix.

timj · 2020-04-20T23:45:53Z

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

+            plugin = self._importPlugin(pluginName)
+            if plugin is None:
+                continue
+            for command in plugin.__all__:


Maybe use:

commands.extend([funcNameToCmdName(command) for command in plugin.__all__])

?

timj · 2020-04-20T23:46:56Z

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

+                    try:
+                        cmd = doImport(fullCommand)
+                    except (TypeError, ModuleNotFoundError, ImportError) as err:
+                        logging.debug("Command import exception: %s", err)


logging -> log (I won't mention again)

timj · 2020-04-20T23:47:54Z

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

+        """
+        try:
+            return doImport(pluginName)
+        except (TypeError, ModuleNotFoundError, ImportError) as err:


Do we gain much by trying to restrict the exception handler to these three rather than always reporting the problem for all Exception?

no, probably not. will change to all Exception.

timj · 2020-04-20T23:53:48Z

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

+        localCmd = self._getLocalCommand(name)
+        if localCmd is not None:
+            return localCmd
+        for pluginName in self._getPluginList():


Would this be simplified a lot of if you had a dict mapping command name to thing to import? You'd presumably then be able to catch people defining two identical subcommand names in different packages. At the moment it seems that if two identical subcommands were defined you'd get the one that was ahead in the environment variable path.

good catch.

timj · 2020-04-20T23:55:32Z

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

+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+__all__ = ["create", "dump_config", "validate_config"]


I think @TallJimbo and I agreed that we wanted these names reversed to be config dump and config validate.

timj · 2020-04-20T23:58:16Z

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

+@repo_option(required=True)
+@click.option("--quiet", "-q", is_flag=True, help="Do not report individual failures.")
+@dataset_type_option(help="Specific DatasetType(s) to validate.")
+@click.option("--ignore", "-i", type=str, multiple=True,


where does the comma splitting happen? Doesn't it need the same callback that dataset types get?

callback splitting is in definition of the dataset-type option. If we need some dataset-type options to take multiple and some not that can be added tot he option. https://github.com/lsst/daf_butler/pull/258/files#diff-191150039ddacf9a20d7744bc23e627aR37

What I'm trying to say is that this --ignore option previously processed commas but I don't see it doing that in this implementation.

Oh, I was looking at the wrong line. I'll add it to ignore

timj · 2020-04-20T23:59:49Z

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

-#!/usr/bin/env python
-
-# This file is part of daf_butler.
+# This file is part of obs_base.


Not obs_base

timj · 2020-04-21T00:01:49Z

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

@@ -0,0 +1,49 @@
+# This file is part of daf_butler.


For utility functions we don't require a separate file per function. The reason I wanted separate files per sub command was that I was worried we were going to get many sub commands and things would get out of control. We envisaged that there would be support code needed for the subcommands and that would get confusing. You seem to have mitigated a lot of that worry with clever callbacks in click definitions but we'll see what happens when more complex commands turn up.

I'll move utils into their own file. I think it's worth keeping commands and options in separate files at least for now.

n8pease commented Apr 13, 2020

View reviewed changes

timj reviewed Apr 14, 2020

View reviewed changes

n8pease force-pushed the tickets/DM-24330 branch from b9c0574 to 696687c Compare April 15, 2020 02:43

timj reviewed Apr 16, 2020

View reviewed changes

n8pease force-pushed the tickets/DM-24330 branch from c2c4fe7 to 509303b Compare April 17, 2020 17:53

timj reviewed Apr 17, 2020

View reviewed changes

n8pease force-pushed the tickets/DM-24330 branch 7 times, most recently from 98a86d4 to 2879484 Compare April 20, 2020 22:43

remove test_scripts

5c0d8ed

n8pease force-pushed the tickets/DM-24330 branch from 2879484 to e2a8432 Compare April 20, 2020 22:55

timj approved these changes Apr 21, 2020

View reviewed changes

n8pease added 4 commits April 21, 2020 11:30

add butler command

9872e21

replace makeButlerRepo with create command

879de71

replace dumpButlerConfig with config-dump

18cf5cf

add dataset option

876587e

n8pease force-pushed the tickets/DM-24330 branch 2 times, most recently from be6a42f to ab3e4f6 Compare April 21, 2020 21:28

n8pease added 3 commits April 21, 2020 17:25

replace validateButlerConfiguration with config-validate

0d6836a

add run_option

7fe815f

add cli documentation

bd84f18

n8pease force-pushed the tickets/DM-24330 branch from ab3e4f6 to bd84f18 Compare April 21, 2020 22:26

n8pease merged commit 0d955ba into master Apr 22, 2020

timj deleted the tickets/DM-24330 branch April 22, 2020 22:00

timj mentioned this pull request Apr 22, 2020

DM-24245: Convert daf_butler script to Click as a test for that package #246

Closed



		@click.group()
		# this group doesn't use the verbose command but does allow (ingore) it,



		def main():
		verbose = len(sys.argv) > 1 and "-v" == sys.argv[1]



		@click.command()
		@repo_option() # required defaul val is False



		def funcNameToCmdName(functionName):
		"""Change underscores, used in fucntions, to dashes, used in commands."""

DM-24330: add ability to run an obs_base command via the butler command #258

DM-24330: add ability to run an obs_base command via the butler command #258

Conversation

n8pease commented Apr 13, 2020

n8pease Apr 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj 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

n8pease Apr 14, 2020 • edited

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

timj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj 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

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 Apr 13, 2020 •

edited

n8pease Apr 14, 2020 •

edited