Skip to content

Commit

Permalink
feat(sdk): improve cli help text (kubeflow#7618)
Browse files Browse the repository at this point in the history
* add docstring parsing utils

* implement docstring parsing for help text

* implement smoke test of all cli commands

* clean up use of optional in docstring

* make command descriptions more consistent

* fix disable_type_check

* update release notes
  • Loading branch information
connor-mccarthy authored and Jagadeesh J committed May 11, 2022
1 parent 7b2bac5 commit a72da08
Show file tree
Hide file tree
Showing 13 changed files with 475 additions and 162 deletions.
2 changes: 2 additions & 0 deletions sdk/RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
* Add pipeline_task_name to PipelineTaskFinalStatus [\#7464](https://github.com/kubeflow/pipelines/pull/7464)
* Depends on `kfp-pipeline-spec>=0.1.14,<0.2.0` [\#7464](https://github.com/kubeflow/pipelines/pull/7464)
* Depends on `google-cloud-storage>=2.2.1,<3` [\#7493](https://github.com/kubeflow/pipelines/pull/7493)
* Add additional methods to `kfp.client.Client` [\#7562](https://github.com/kubeflow/pipelines/pull/7562), [\#7463](https://github.com/kubeflow/pipelines/pull/7463)
* Migrate V1 CLI to V2, with improvements [\#7547](https://github.com/kubeflow/pipelines/pull/7547), [\#7558](https://github.com/kubeflow/pipelines/pull/7558), [\#7559](https://github.com/kubeflow/pipelines/pull/7559), [\#7560](https://github.com/kubeflow/pipelines/pull/7560), , [\#7569](https://github.com/kubeflow/pipelines/pull/7569), [\#7567](https://github.com/kubeflow/pipelines/pull/7567), [\#7603](https://github.com/kubeflow/pipelines/pull/7603), [\#7606](https://github.com/kubeflow/pipelines/pull/7606), [\#7607](https://github.com/kubeflow/pipelines/pull/7607), [\#7628](https://github.com/kubeflow/pipelines/pull/7628), [\#7618](https://github.com/kubeflow/pipelines/pull/7618)

## Documentation Updates

Expand Down
23 changes: 12 additions & 11 deletions sdk/python/kfp/cli/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import click
import kfp
from kfp import client
from kfp.cli import component
from kfp.cli import diagnose_me_cli
from kfp.cli import dsl
Expand All @@ -26,7 +27,7 @@
from kfp.cli import run
from kfp.cli.output import OutputFormat
from kfp.cli.utils import aliased_plurals_group
from kfp.client import Client
from kfp.cli.utils import parsing

COMMANDS = {
'client': {
Expand Down Expand Up @@ -69,20 +70,21 @@ def _install_completion(shell: str) -> None:
'--install-completion',
type=click.Choice(list(SHELL_FILES.keys())),
default=None)
@click.option('--endpoint', help='Endpoint of the KFP API service to connect.')
@click.option('--iap-client-id', help='Client ID for IAP protected endpoint.')
@click.option('--endpoint', help=parsing.get_param_descr(client.Client, 'host'))
@click.option(
'--iap-client-id', help=parsing.get_param_descr(client.Client, 'client_id'))
@click.option(
'-n',
'--namespace',
default='kubeflow',
show_default=True,
help='Kubernetes namespace to connect to the KFP API.')
help=parsing.get_param_descr(client.Client, 'namespace'))
@click.option(
'--other-client-id',
help='Client ID for IAP protected endpoint to obtain the refresh token.')
help=parsing.get_param_descr(client.Client, 'other_client_id'))
@click.option(
'--other-client-secret',
help='Client ID for IAP protected endpoint to obtain the refresh token.')
help=parsing.get_param_descr(client.Client, 'other_client_secret'))
@click.option(
'--output',
type=click.Choice(list(map(lambda x: x.name, OutputFormat))),
Expand All @@ -94,10 +96,9 @@ def _install_completion(shell: str) -> None:
def cli(ctx: click.Context, endpoint: str, iap_client_id: str, namespace: str,
other_client_id: str, other_client_secret: str, output: OutputFormat,
show_completion: str, install_completion: str):
"""kfp is the command line interface to KFP service.
"""Kubeflow Pipelines CLI.
Feature stage:
[Alpha](https://github.com/kubeflow/pipelines/blob/07328e5094ac2981d3059314cc848fbb71437a76/docs/release/feature-stages.md#alpha)
The KFP CLI is currently in Alpha feature stage (https://github.com/kubeflow/pipelines/blob/07328e5094ac2981d3059314cc848fbb71437a76/docs/release/feature-stages.md#alpha).
"""
if show_completion:
click.echo(_create_completion(show_completion))
Expand All @@ -114,7 +115,7 @@ def cli(ctx: click.Context, endpoint: str, iap_client_id: str, namespace: str,
if ctx.invoked_subcommand not in client_commands:
# Do not create a client for these subcommands
return
ctx.obj['client'] = Client(endpoint, iap_client_id, namespace,
other_client_id, other_client_secret)
ctx.obj['client'] = client.Client(endpoint, iap_client_id, namespace,
other_client_id, other_client_secret)
ctx.obj['namespace'] = namespace
ctx.obj['output'] = output
31 changes: 31 additions & 0 deletions sdk/python/kfp/cli/cli_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from typing import Any, Dict, List, Optional
from unittest import mock

import click
import yaml
from absl.testing import parameterized
from click import testing
Expand Down Expand Up @@ -256,5 +257,35 @@ def test_deprecated_command_is_found(self):
self.assertEqual(result.exit_code, 0)


info_dict = cli.cli.to_info_dict(ctx=click.Context(cli.cli))
commands_dict = {
command: list(body.get('commands', {}).keys())
for command, body in info_dict['commands'].items()
}
noun_verb_list = [
(noun, verb) for noun, verbs in commands_dict.items() for verb in verbs
]


class TestSmokeTestAllCommandsWithHelp(parameterized.TestCase):

@classmethod
def setUpClass(cls):
cls.runner = testing.CliRunner()

cls.vals = [('run', 'list')]

@parameterized.parameters(*noun_verb_list)
def test(self, noun: str, verb: str):
with mock.patch('kfp.cli.cli.client.Client'):
result = self.runner.invoke(
args=[noun, verb, '--help'],
cli=cli.cli,
catch_exceptions=False,
obj={})
self.assertTrue(result.output.startswith('Usage: '))
self.assertEqual(result.exit_code, 0)


if __name__ == '__main__':
unittest.main()
1 change: 1 addition & 0 deletions sdk/python/kfp/cli/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ def build(components_directory: pathlib.Path, component_filepattern: str,
engine: str, kfp_package_path: Optional[pathlib.Path],
overwrite_dockerfile: bool, push_image: bool):
"""Builds containers for KFP v2 Python-based components."""

if engine != 'docker':
warnings.warn(
'The --engine option is deprecated and does not need to be passed. Only Docker engine is supported and will be used by default.',
Expand Down
9 changes: 3 additions & 6 deletions sdk/python/kfp/cli/diagnose_me_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
from typing import Dict, List, Text, Union

import click
from kfp.cli.diagnose_me import dev_env, gcp
from kfp.cli.diagnose_me import dev_env
from kfp.cli.diagnose_me import gcp
from kfp.cli.diagnose_me import kubernetes_cluster
from kfp.cli.diagnose_me import kubernetes_cluster as k8
from kfp.cli.diagnose_me import utility
Expand Down Expand Up @@ -34,11 +35,7 @@
@click.pass_context
def diagnose_me(ctx: click.Context, json: bool, project_id: str,
namespace: str):
"""Runs KFP environment diagnostic with specified parameters.
Feature stage:
[Alpha](https://github.com/kubeflow/pipelines/blob/07328e5094ac2981d3059314cc848fbb71437a76/docs/release/feature-stages.md#alpha)
"""
"""Runs KFP environment diagnostic."""
# validate kubectl, gcloud , and gsutil exist
local_env_gcloud_sdk = gcp.get_gcp_configuration(
gcp.Commands.GET_GCLOUD_VERSION,
Expand Down
26 changes: 14 additions & 12 deletions sdk/python/kfp/cli/dsl_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import click
from kfp import compiler
from kfp.cli.utils import parsing
from kfp.components import pipeline_context


Expand All @@ -28,17 +29,17 @@ def _compile_pipeline_function(
function_name: Optional[str],
pipeline_parameters: Optional[Mapping[str, Any]],
package_path: str,
type_check: bool,
disable_type_check: bool,
) -> None:
"""Compiles a pipeline function.
Args:
pipeline_funcs: A list of pipeline_functions.
function_name: The name of the pipeline function to compile if there were
function_name: The name of the pipeline function to compile if there are
multiple.
pipeline_parameters: The pipeline parameters as a dict of {name: value}.
package_path: The output path of the compiled result.
type_check: Whether to enable the type checking.
pipeline_parameters: The pipeline parameters as a dictionary.
package_path: Path to write the compiled result.
disable_type_check: Whether to disable type checking.
"""
if len(pipeline_funcs) == 0:
raise ValueError(
Expand All @@ -65,7 +66,7 @@ def _compile_pipeline_function(
pipeline_func=pipeline_func,
pipeline_parameters=pipeline_parameters,
package_path=package_path,
type_check=type_check)
type_check=not disable_type_check)


class PipelineCollectorContext():
Expand Down Expand Up @@ -93,13 +94,13 @@ def __exit__(self, *args):
'--output',
type=str,
required=True,
help='Local path to the output PipelineJob JSON file.')
help=parsing.get_param_descr(_compile_pipeline_function, 'package_path'))
@click.option(
'--function',
'function_name',
type=str,
default=None,
help='The name of the function to compile if there are multiple.')
help=parsing.get_param_descr(_compile_pipeline_function, 'function_name'))
@click.option(
'--pipeline-parameters',
type=str,
Expand All @@ -109,13 +110,14 @@ def __exit__(self, *args):
'--disable-type-check',
is_flag=True,
default=False,
help='Disable the type check. Default: type check is not disabled.')
help=parsing.get_param_descr(_compile_pipeline_function,
'disable_type_check'))
def dsl_compile(
py: str,
output: str,
function_name: Optional[str] = None,
pipeline_parameters: str = None,
disable_type_check: bool = True,
disable_type_check: bool = False,
) -> None:
"""Compiles a pipeline written in a .py file."""
sys.path.insert(0, os.path.dirname(py))
Expand All @@ -128,15 +130,15 @@ def dsl_compile(
pipeline_parameters) if pipeline_parameters is not None else {}
except json.JSONDecodeError as e:
logging.error(
f"Failed to parse --pipeline-parameters argument: {pipeline_parameters}"
f'Failed to parse --pipeline-parameters argument: {pipeline_parameters}'
)
raise e
_compile_pipeline_function(
pipeline_funcs=pipeline_funcs,
function_name=function_name,
pipeline_parameters=parsed_parameters,
package_path=output,
type_check=not disable_type_check,
disable_type_check=disable_type_check,
)
finally:
del sys.path[0]
Expand Down
42 changes: 26 additions & 16 deletions sdk/python/kfp/cli/experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@

import click
import kfp_server_api
from kfp import client
from kfp.cli.output import OutputFormat
from kfp.cli.output import print_output
from kfp.cli.utils import parsing
from kfp_server_api.models.api_experiment import ApiExperiment


Expand All @@ -15,7 +17,11 @@ def experiment():


@experiment.command()
@click.option('-d', '--description', help='Description of the experiment.')
@click.option(
'-d',
'--description',
help=parsing.get_param_descr(client.Client.create_experiment,
'description'))
@click.argument('name')
@click.pass_context
def create(ctx: click.Context, description: str, name: str):
Expand All @@ -30,20 +36,21 @@ def create(ctx: click.Context, description: str, name: str):

@experiment.command()
@click.option(
'--page-token', default='', help='Token for starting of the page.')
'--page-token',
default='',
help=parsing.get_param_descr(client.Client.list_experiments, 'page_token'))
@click.option(
'-m', '--max-size', default=100, help='Max size of the listed experiments.')
'-m',
'--max-size',
default=100,
help=parsing.get_param_descr(client.Client.list_experiments, 'page_size'))
@click.option(
'--sort-by',
default='created_at desc',
help="Can be '[field_name]', '[field_name] desc'. For example, 'name desc'."
)
help=parsing.get_param_descr(client.Client.list_experiments, 'sort_by'))
@click.option(
'--filter',
help=(
'filter: A url-encoded, JSON-serialized Filter protocol buffer '
'(see [filter.proto](https://github.com/kubeflow/pipelines/blob/master/backend/api/filter.proto)).'
))
help=parsing.get_param_descr(client.Client.list_experiments, 'filter'))
@click.pass_context
def list(ctx: click.Context, page_token: str, max_size: int, sort_by: str,
filter: str):
Expand All @@ -70,7 +77,7 @@ def list(ctx: click.Context, page_token: str, max_size: int, sort_by: str,
@click.argument('experiment-id')
@click.pass_context
def get(ctx: click.Context, experiment_id: str):
"""Get detailed information about an experiment."""
"""Get information about an experiment."""
client = ctx.obj['client']
output_format = ctx.obj['output']

Expand Down Expand Up @@ -120,17 +127,20 @@ def _display_experiment(exp: kfp_server_api.ApiExperiment,
print_output(dict(table), [], output_format)


either_option_required = 'Either --experiment-id or --experiment-name is required.'


@experiment.command()
@click.option(
'--experiment-id',
default=None,
help='The ID of the experiment to archive, can only supply either an experiment ID or name.'
help=parsing.get_param_descr(client.Client.archive_experiment,
'experiment_id') + ' ' + either_option_required
)
@click.option(
'--experiment-name',
default=None,
help='The name of the experiment to archive, can only supply either an experiment ID or name.'
)
help='Name of the experiment.' + ' ' + either_option_required)
@click.pass_context
def archive(ctx: click.Context, experiment_id: str, experiment_name: str):
"""Archive an experiment."""
Expand All @@ -152,13 +162,13 @@ def archive(ctx: click.Context, experiment_id: str, experiment_name: str):
@click.option(
'--experiment-id',
default=None,
help='The ID of the experiment to archive, can only supply either an experiment ID or name.'
help=parsing.get_param_descr(client.Client.unarchive_experiment,
'experiment_id') + ' ' + either_option_required
)
@click.option(
'--experiment-name',
default=None,
help='The name of the experiment to archive, can only supply either an experiment ID or name.'
)
help='Name of the experiment.' + ' ' + either_option_required)
@click.pass_context
def unarchive(ctx: click.Context, experiment_id: str, experiment_name: str):
"""Unarchive an experiment."""
Expand Down
Loading

0 comments on commit a72da08

Please sign in to comment.