From 78199b5789b16c642c3a9d4864cd8494db3e1398 Mon Sep 17 00:00:00 2001 From: Sergey Serebryakov Date: Mon, 11 Dec 2023 15:38:31 -0800 Subject: [PATCH] Refactoring code related to MLCube CLI options. (#350) - Removing deprecated option definitions from `__main__.py` (new options are defined in `Options` class). - Moving new option definitions (network, security etc.) to `Options` class. - Removing unused and incorrect code related to parsing options accepting multiple values from the `__main__.py` file since this functionality is implemented in `cli` module. In addition, updating the changed files to comply with python style guidelines. --- mlcube/mlcube/__main__.py | 125 ++------------------------------ mlcube/mlcube/cli.py | 102 +++++++++++++++----------- mlcube/mlcube/tests/test_cli.py | 27 ++++--- 3 files changed, 80 insertions(+), 174 deletions(-) diff --git a/mlcube/mlcube/__main__.py b/mlcube/mlcube/__main__.py index 26757d85..4c1f6104 100644 --- a/mlcube/mlcube/__main__.py +++ b/mlcube/mlcube/__main__.py @@ -10,14 +10,7 @@ import coloredlogs from omegaconf import OmegaConf -from mlcube.cli import ( - MLCubeCommand, - MultiValueOption, - Options, - UsageExamples, - parse_cli_args, -) -from mlcube.config import MountType +from mlcube.cli import MLCubeCommand, MultiValueOption, Options, UsageExamples, parse_cli_args from mlcube.errors import ExecutionError, IllegalParameterValueError, MLCubeError from mlcube.parser import CliParser from mlcube.shell import Shell @@ -29,110 +22,6 @@ """Width of a user terminal. MLCube overrides default (80) character width to make usage examples look better.""" -def add_to_parser(self, parser: click.parser.OptionParser, ctx: click.core.Context): - def parser_process(value: str, state: click.parser.ParsingState): - values: t.List[str] = [value] - prefixes: t.Tuple[str] = tuple(self._eat_all_parser.prefixes) - while state.rargs: - if state.rargs[0].startswith(prefixes): - break - values.append(state.rargs.pop(0)) - self._previous_parser_process(tuple(values), state) - - super(MultiValueOption, self).add_to_parser(parser, ctx) - for opt_name in self.opts: - our_parser: t.Optional[click.parser.Option] = parser._long_opt.get(opt_name) or parser._short_opt.get(opt_name) - if our_parser: - self._eat_all_parser = our_parser - self._previous_parser_process = our_parser.process - our_parser.process = parser_process - break - - -log_level_option = click.option( - "--log-level", - "--log_level", - required=False, - type=str, - default="warning", - help="Log level to set, default is to do nothing.", -) -mlcube_option = click.option( - "--mlcube", - required=False, - type=str, - default=os.getcwd(), - help="Path to MLCube. This can be either a directory path that becomes MLCube's root directory, or path to MLCube" - "definition file (.yaml). In the latter case the MLCube's root directory becomes parent directory of the yaml" - "file. Default is current directory.", -) -platform_option = click.option( - "--platform", - required=False, - type=str, - default="docker", - help="Platform to run MLCube, default is 'docker' (that also supports podman).", -) -task_option = click.option( - "--task", - required=False, - type=str, - default=None, - help="MLCube task name(s) to run, default is `main`. This parameter can take a list value, in which case task names" - "are separated with ','.", -) -workspace_option = click.option( - "--workspace", - required=False, - type=str, - default=None, - help="Workspace location that is used to store input/output artifacts of MLCube tasks.", -) -network_option = click.option( - "--network", - required=False, - type=str, - default=None, - help="Networking options defined during MLCube container execution.", -) -security_option = click.option( - "--security", - required=False, - type=str, - default=None, - help="Security options defined during MLCube container execution.", -) -gpus_option = click.option( - "--gpus", - required=False, - type=str, - default=None, - help="GPU usage options defined during MLCube container execution.", -) -memory_option = click.option( - "--memory", - required=False, - type=str, - default=None, - help="Memory RAM options defined during MLCube container execution.", -) -cpu_option = click.option( - "--cpu", - required=False, - type=str, - default=None, - help="CPU options defined during MLCube container execution.", -) -mount_option = click.option( - "--mount", - required=False, - type=click.Choice([MountType.RW, MountType.RO]), - default=None, - help="Mount options for all input parameters. These mount options override any other mount options defined for " - "each input parameters. A typical use case is to ensure that inputs are mounted in read-only (ro) mode.", -) - - @click.group(name="mlcube", add_help_option=False) @Options.loglevel @Options.help @@ -280,12 +169,12 @@ def configure(mlcube: t.Optional[str], platform: str, p: t.Tuple[str]) -> None: @Options.platform @Options.task @Options.workspace -@network_option -@security_option -@gpus_option -@memory_option -@cpu_option -@mount_option +@Options.network +@Options.security +@Options.gpus +@Options.memory +@Options.cpu +@Options.mount @Options.parameter @Options.help @click.pass_context diff --git a/mlcube/mlcube/cli.py b/mlcube/mlcube/cli.py index 9c38da1e..1f428ad8 100644 --- a/mlcube/mlcube/cli.py +++ b/mlcube/mlcube/cli.py @@ -15,7 +15,7 @@ from markdown import Markdown from omegaconf import DictConfig -from mlcube.config import MLCubeConfig +from mlcube.config import MLCubeConfig, MountType from mlcube.parser import CliParser, MLCubeDirectory from mlcube.platform import Platform from mlcube.runner import Runner @@ -54,15 +54,11 @@ def parse_cli_args( mlcube_inst: MLCubeDirectory = CliParser.parse_mlcube_arg(parsed_args["mlcube"]) Validate.validate_type(mlcube_inst, MLCubeDirectory) - mlcube_cli_args, task_cli_args = CliParser.parse_extra_arg( - unparsed_args, parsed_args - ) + mlcube_cli_args, task_cli_args = CliParser.parse_extra_arg(unparsed_args, parsed_args) if parsed_args.get("platform", None) is not None: system_settings = SystemSettings() - runner_config: t.Optional[DictConfig] = system_settings.get_platform( - parsed_args["platform"] - ) + runner_config: t.Optional[DictConfig] = system_settings.get_platform(parsed_args["platform"]) runner_cls: t.Optional[t.Type[Runner]] = Platform.get_runner( system_settings.runners.get(runner_config.runner, None) ) @@ -147,9 +143,9 @@ def parser_process(value: str, state: click.parser.ParsingState): super(MultiValueOption, self).add_to_parser(parser, ctx) for opt_name in self.opts: - our_parser: t.Optional[click.parser.Option] = parser._long_opt.get( + our_parser: t.Optional[click.parser.Option] = parser._long_opt.get(opt_name) or parser._short_opt.get( opt_name - ) or parser._short_opt.get(opt_name) + ) if our_parser: self._eat_all_parser = our_parser self._previous_parser_process = our_parser.process @@ -171,9 +167,7 @@ class HelpEpilog(object): class Example: """Context manager for helping with formatting epilogues when users invoke help on a command line.""" - def __init__( - self, formatter: click.formatting.HelpFormatter, title: str - ) -> None: + def __init__(self, formatter: click.formatting.HelpFormatter, title: str) -> None: self.formatter = formatter self.title = title @@ -191,9 +185,7 @@ def __exit__(self, exc_type, exc_val, exc_tb) -> None: def __init__(self, examples: t.List[t.Tuple[str, t.List[str]]]) -> None: self.examples = examples - def format_epilog( - self, ctx: click.core.Context, formatter: click.formatting.HelpFormatter - ) -> None: + def format_epilog(self, ctx: click.core.Context, formatter: click.formatting.HelpFormatter) -> None: if not self.examples: return formatter.write_heading("\nExamples") @@ -233,7 +225,7 @@ def format_help_text(self, ctx, formatter): if start_idx >= 0: end_idx: int = text.find("", start_idx) if end_idx >= 0: - text = text[:start_idx] + text[end_idx + 7:] + text = text[:start_idx] + text[end_idx + 7 :] # This is supported in default implementation, so need to reimplement it here. All that goes after the # `\f` character is not rendered (e.g., description of parameters, TODOs, etc.). text = text.split("\f")[0] @@ -244,9 +236,7 @@ def format_help_text(self, ctx, formatter): if len(paragraphs) == 1: brief_description, long_description = paragraphs[0], "" else: - brief_description, long_description = paragraphs[0], "\n".join( - paragraphs[1:] - ) + brief_description, long_description = paragraphs[0], "\n".join(paragraphs[1:]) formatter.write_heading("\nBrief description") with formatter.indentation(): @@ -265,9 +255,7 @@ def format_help_text(self, ctx, formatter): with formatter.indentation(): formatter.write_text(DEPRECATED_HELP_NOTICE) - def format_options( - self, ctx: click.core.Context, formatter: click.formatting.HelpFormatter - ) -> None: + def format_options(self, ctx: click.core.Context, formatter: click.formatting.HelpFormatter) -> None: """Writes all the options into the formatter if they exist. This implementation removes Markdown format from the options' help messages should they exist. Any errors @@ -283,9 +271,7 @@ def format_options( with formatter.section("Options"): formatter.write_dl(opts) - def format_epilog( - self, ctx: click.core.Context, formatter: click.formatting.HelpFormatter - ) -> None: + def format_epilog(self, ctx: click.core.Context, formatter: click.formatting.HelpFormatter) -> None: """Format epilog if its type `mlcube.EpilogWithExamples`, else fallback to default implementation.""" if isinstance(self.epilog, HelpEpilog): self.epilog.format_epilog(ctx, formatter) @@ -332,9 +318,7 @@ class Options: type=click.Choice(["critical", "error", "warning", "info", "debug"]), help="Logging level is a lower-case string value for Python's logging library (see " "[Logging Levels]({log_level}) for more details). Only messages with this logging level or higher are " - "logged.".format( - log_level="https://docs.python.org/3/library/logging.html#logging-levels" - ), + "logged.".format(log_level="https://docs.python.org/3/library/logging.html#logging-levels"), ) mlcube = click.option( @@ -382,9 +366,7 @@ class Options: type=str, default=None, help="MLCube [task]({task}) name(s) to run, default is `main`. This parameter can take a list of values, in " - "which case task names are separated with comma (,).".format( - task=OnlineDocs.concept_url("task") - ), + "which case task names are separated with comma (,).".format(task=OnlineDocs.concept_url("task")), ) workspace = click.option( @@ -434,6 +416,50 @@ class Options: ), ) + network = click.option( + "--network", + required=False, + type=str, + default=None, + help="Networking options defined during MLCube container execution.", + ) + security = click.option( + "--security", + required=False, + type=str, + default=None, + help="Security options defined during MLCube container execution.", + ) + gpus = click.option( + "--gpus", + required=False, + type=str, + default=None, + help="GPU usage options defined during MLCube container execution.", + ) + memory = click.option( + "--memory", + required=False, + type=str, + default=None, + help="Memory RAM options defined during MLCube container execution.", + ) + cpu = click.option( + "--cpu", + required=False, + type=str, + default=None, + help="CPU options defined during MLCube container execution.", + ) + mount = click.option( + "--mount", + required=False, + type=click.Choice([MountType.RW, MountType.RO]), + default=None, + help="Mount options for all input parameters. These mount options override any other mount options defined for " + "each input parameters. A typical use case is to ensure that inputs are mounted in read-only (ro) mode.", + ) + def _mnist(steps: t.List[str]) -> t.List[str]: return [ @@ -451,9 +477,7 @@ class UsageExamples: ), ( "Show effective MLCube configuration overriding parameters on a command line", - _mnist( - ["mlcube show_config --mlcube=mnist -Pdocker.build_strategy=auto"] - ), + _mnist(["mlcube show_config --mlcube=mnist -Pdocker.build_strategy=auto"]), ), ] ) @@ -473,19 +497,13 @@ class UsageExamples: [ ( "Run MNIST MLCube project", - _mnist( - [ - "mlcube run --mlcube=mnist --platform=docker --task=download,train" - ] - ), + _mnist(["mlcube run --mlcube=mnist --platform=docker --task=download,train"]), ) ] ) """Usage examples for `mlcube run` command.""" - describe = HelpEpilog( - [("Run MNIST MLCube project", _mnist(["mlcube describe --mlcube=mnist"]))] - ) + describe = HelpEpilog([("Run MNIST MLCube project", _mnist(["mlcube describe --mlcube=mnist"]))]) """Usage examples for `mlcube describe` command.""" create = HelpEpilog([("Create a new empty MLCube project", ["mlcube create"])]) diff --git a/mlcube/mlcube/tests/test_cli.py b/mlcube/mlcube/tests/test_cli.py index b8d8a6fd..e8e1cfa5 100644 --- a/mlcube/mlcube/tests/test_cli.py +++ b/mlcube/mlcube/tests/test_cli.py @@ -1,11 +1,11 @@ import typing as t from unittest import TestCase -from click import Option, BaseCommand +from click import BaseCommand, Option from click.testing import CliRunner, Result -from mlcube.cli import (markdown2text, Options) -from mlcube.__main__ import cli, show_config, configure, run, describe, config, create +from mlcube.__main__ import cli, config, configure, create, describe, run, show_config +from mlcube.cli import Options, markdown2text class TestCli(TestCase): @@ -14,34 +14,33 @@ class Obj: ... decorators: t.List[str] = [] - for name in (v for v, m in vars(Options).items() if not v.startswith(('_', '__')) and callable(m)): + for name in (v for v, m in vars(Options).items() if not v.startswith(("_", "__")) and callable(m)): decorator: t.Optional[t.Callable] = getattr(Options, name, None) self.assertIsNotNone(decorator, f"MLCube BUG: option decorator must not be None (name = {name}).") obj = decorator(Obj()) - params: t.List = getattr(obj, '__click_params__', None) + params: t.List = getattr(obj, "__click_params__", None) self.assertIsInstance(params, list, f"Click library changed its implementation? Type={type(params)}.") self.assertEqual(1, len(params), f"Click library changed its implementation? Size={len(params)}.") cmd: Option = params[0] - self.assertTrue( - isinstance(cmd.help, str) and len(cmd.help) > 0, - f"Invalid option help ({cmd.help})" - ) + self.assertTrue(isinstance(cmd.help, str) and len(cmd.help) > 0, f"Invalid option help ({cmd.help})") plain_text = markdown2text(cmd.help) self.assertTrue( isinstance(plain_text, str) and len(plain_text) > 0, - f"Invalid markdown2text conversion from markdown ('{cmd.help}') to plain text ('{plain_text}')." + f"Invalid markdown2text conversion from markdown ('{cmd.help}') to plain text ('{plain_text}').", ) decorators.append(name) + expected_options_count: int = 14 self.assertEqual( - 8, len(decorators), f"Expected number of option decorators is 8. Actual tested decorators: {decorators}." + expected_options_count, + len(decorators), + f"Expected number of option decorators is {expected_options_count}. " + f"Actual tested decorators: {decorators}.", ) def test_help(self) -> None: """python -m unittest mlcube.tests.test_cli""" - cli_funcs = [ - cli, show_config, configure, run, describe, config, create - ] + cli_funcs = [cli, show_config, configure, run, describe, config, create] for cli_func in cli_funcs: self.assertIsInstance(cli_func, BaseCommand) result: Result = CliRunner().invoke(cli_func, [f"--help"])