DM-25385 make a pipetask command based on click #318
Conversation
There was a problem hiding this comment.
Looks okay. A comment in the pull request description providing an overview of the changes would have helped me a little since the PR title doesn't help (I really need to take another look at that dev guide rule) and the commit messages are very terse. I think what you've done here is rearrange the code a bit into a class so that the pipetask infrastructure can use it.
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| class LoaderCLI(click.MultiCommand, abc.ABC): |
There was a problem hiding this comment.
I think it needs docstring, it's hard to guess what its purpose is.
python/lsst/daf/butler/cli/butler.py
Outdated
| """Convert function name to the butler command name: change | ||
| underscores, (used in functions) to dashes (used in commands), and | ||
| change local-package command names that conflict with python keywords | ||
| to a leagal function name. |
| @classmethod | ||
| def _funcNameToCmdName(cls, functionName): | ||
| """Convert function name to the butler command name: change | ||
| underscores, (used in functions) to dashes (used in commands), and |
There was a problem hiding this comment.
No need for comma after "underscores"?
python/lsst/daf/butler/cli/butler.py
Outdated
| """Convert function name to the butler command name: change | ||
| underscores, (used in functions) to dashes (used in commands), and | ||
| change local-package command names that conflict with python keywords | ||
| to a leagal function name. |
There was a problem hiding this comment.
I don't think you need to repeat whole docstring for the method that you are overriding, just add a comment like this:
https://github.com/lsst/daf_butler/blob/master/python/lsst/daf/butler/datastores/chainedDatastore.py#L354
| required : bool, optional | ||
| If true, the option is required to be passed in on the command line, by | ||
| default False | ||
| split_kv_sep : `str` or None, optional |
There was a problem hiding this comment.
There is split_kv parameter but no split_kv_sep.
|
|
||
| Returns | ||
| ------- | ||
| cli : a click.Command function. |
There was a problem hiding this comment.
Is "a click.Command function" different from `click.Command`?
There was a problem hiding this comment.
In this case click.Command is more correct. By "command function" I mean the function decorated by @click.Command, whereas a click.Command instance would wrap & contain a command function.
| invoker. | ||
| """ | ||
| cliArgs = copy.copy(parameterKwargs) if parameterKwargs is not None else {} | ||
| if 'parameterType' not in cliArgs and optTestBase.isArgument and optTestBase.isParameter: |
There was a problem hiding this comment.
Strings need double quotes (many other places too).
| @abc.abstractmethod | ||
| def optionName(self): | ||
| """The option name, matches the option flag that appears on the command | ||
| line |
There was a problem hiding this comment.
Need more periods everywhere at the end of sentences.
| if testObj.valueType.dir_okay: | ||
| os.makedirs(testObj.optionValue) | ||
| elif testObj.valueType.file_okay: | ||
| _ = open(testObj.optionValue) |
There was a problem hiding this comment.
My guess is that you are creating a file here, should this be open(..., "w")? And I think it needs an explicit close() or with open(...).
| @@ -55,9 +55,9 @@ def test_requiredMissing(self): | |||
| def test_all(self): | |||
| """Test all parameters.""" | |||
| self.run_test(["query-dataset-types", "here", "--verbose", "foo*", "--components"], | |||
| self.makeExpected(repo="here", verbose=True, glob=("foo*", ), components=True)) | |||
| self.makeExpected(repo="here", verbose=True, glob=["foo*", ], components=True)) | |||
There was a problem hiding this comment.
Can remove comma too to save two characters 🙂 (and below too)
n8pease
force-pushed
the
tickets/DM-25385
branch
3 times, most recently
from
a57885e
to
ca0d8a9
Compare
python/lsst/daf/butler/cli/log.py
Outdated
|
|
||
| # Forward all Python logging to lsstLog | ||
| lgr.addHandler(lsstLog.LogHandler()) | ||
| lgr.setLevel(logging.INFO) |
There was a problem hiding this comment.
If lsstLog is None then I think we probably need to call logging.basicConfig(level=INFO) instead of just lgr.setLevel(logging.INFO).
python/lsst/daf/butler/cli/log.py
Outdated
| def uninitLog(cls, components): | ||
| if lsstLog is not None: | ||
| lgr = logging.getLogger() | ||
| lgr.removeHandler(lsstLog.LogHandler()) |
There was a problem hiding this comment.
Hmm 🤔 I'm not sure it is going to work. Do you have unit test for this?
There was a problem hiding this comment.
@andy-slac can you say more about what part won't work? What would you like tested?
There was a problem hiding this comment.
What I think is that removeHandler removes existing handler from the the handler list. lsstLog.LogHandler() make a new handler that was never added to that list, so removing it does not even make sense (it is probably no-op). What you want is to remove handler instance that you created earlier and added with addHandler , this means you need to remember that instance somewhere. As for testing, I think you can check the size of the lgr.handlers to see if removing changes its size (though handlers is an undocumented attribute)
There was a problem hiding this comment.
You can't test in daf_butler itself because daf_butler doesn't depend on lsst.log.
There was a problem hiding this comment.
You can if you (accidentally) setup log or some other package that depends on log.
There was a problem hiding this comment.
That is true but you can't rely on it from a Jenkins point of view. ctrl_mpexec can test it though.
There was a problem hiding this comment.
I have tests in obs_base for features in daf_butler that depend on lsst.log. Let me know if you think that's not a good place.
I don't think I can depend on the actual number of handlers, tests may run before this test may affect it. I'm not even sure I can depend on a delta. I might have to figure out how to tell if a specific handler is in the list, or punt on being able to know.
The affect that persistent logging configuration has on unrelated unit tests that run in the same process is really giving me hell. I might have questions in Slack today.
There was a problem hiding this comment.
obs_base is good. Logging does affect global state which is why logger configurations should only be triggered by command line tooling and not by module code.
python/lsst/daf/butler/cli/log.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| longlog : `bool` |
There was a problem hiding this comment.
NSP - no such parameter :)
python/lsst/daf/butler/cli/log.py
Outdated
| logLevels = logLevels.items() | ||
|
|
||
| # configure individual loggers | ||
| for component, level in logLevels: |
There was a problem hiding this comment.
DO you need to do level = level.upper()?
n8pease
force-pushed
the
tickets/DM-25385
branch
2 times, most recently
from
ea759ef
to
23701d3
Compare
This allows the LoaderCLI class to be used for the butler command as well as other commands (currently implementing `pipetask`).
The traceback helps with debugging unit tests.
contians fixes found during conversion
Init'ing logging before trying to get commands or a command allows logging info about errors encountered during those actions.
when an option that accepts a key-value input that does not accept multiple values gets a comma-separated value, give an accurate error report.
This helps with click help text when it's long, because the formatting preserved in the triple-quoted string conflicts with the line wrapping in click's help text output.
Allows commands other than 'butler' to be tested (currently working on 'pipetask').
The name of the environment variable that caused the butler command to call a mock instead of a script (command implementation) function was butler-specific. Renaming to be more correct when mocking commands other than butler (currently working on pipetask).
Add MWOption and MWArgument (MW = "middleware") for metavar formatting, can be used for other parameter overrides as needed. The override added in this commit makes sure that the " ..." after the type metavar is present in all the cases where multiple arguments are accepted.
* change log-level to optionally accept key-value pairs * configure python and C++ logging * init (add handler) to the log once * log level can be set on modules or globally at any time * add a logging unit test Changes the type passed from the log-level option log_level to be a dict, any non-kv value is put in the dict in an entry whose key is an empty string. If any keys are repeated (including no key, in which case the key will be an empty string) then the last entry takes precedence. Applies the Choice (accepted input values) to the value; any key will be accepted. add logging unit test
instead of a list.
use mock to verify expected call instead of printing to and reading from stdout. also use setUp to create the click test runner.
This context manager helps tests publish command plugins temporarily.
This allows click's CliRunner in unit tests to execute commands that affect the log settings, and undoes those settings so that the logging system is not left in an unexpected state after running CLI unit tests in the same process as other unit tests.
test_cliOption.py was added to remove redundancy in option tests, factor test_cliArgumentRepo.py into test_cliOption to take advantage of all the test reuse there.
create is covered in test_cliCmdCreate.py
test_cliOption.py was added to remove redundancy in option tests, factor test_cliOptionTransfer.py into test_cliOption to take advantage of all the test reuse there.
LogCliRunner extends CliRunner to reset the CliLog at the end of invoke.
n8pease
force-pushed
the
tickets/DM-25385
branch
from
23701d3
to
82c217a
Compare
No description provided.