kubeflow
/
pipelines
mirror of https://github.com/kubeflow/pipelines.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(sdk): some cli fixes (#7668) 

* use correct type when option is path

* fix options on create recurring-run

* make output logging more consistent

* show deprecation warning for dsl-compile on invocation

* fix unarchive experiment

* fix broken f-string

* convert camelcase methods to snake case
This commit is contained in:
Connor McCarthy 2022-05-05 12:47:18 -06:00 committed by GitHub
parent 8b1155158e
commit 18f0842ee0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 101 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
sdk/python/kfp/cli/component.py
View File

@ -159,7 +159,7 @@ class ComponentBuilder():
def _load_components(self):
if not self._component_files:
logging.error(
'No component files found matching pattern `{self._component_filepattern}` in directory {self._context_directory}'
f'No component files found matching pattern `{self._component_filepattern}` in directory {self._context_directory}'
)
raise sys.exit(1)
@ -330,9 +330,7 @@ def component():
@component.command()
@click.argument(
'components_directory',
type=pathlib.Path,
)
'components_directory', type=click.Path(exists=True, file_okay=False))
@click.option(
'--component-filepattern',
type=str,
@ -346,7 +344,7 @@ def component():
help="Engine to use to build the component's container.")
@click.option(
'--kfp-package-path',
type=pathlib.Path,
type=click.Path(exists=True),
default=None,
help='A pip-installable path to the KFP package.')
@click.option(
@ -362,9 +360,9 @@ def component():
is_flag=True,
default=True,
help='Push the built image to its remote repository.')
def build(components_directory: pathlib.Path, component_filepattern: str,
engine: str, kfp_package_path: Optional[pathlib.Path],
overwrite_dockerfile: bool, push_image: bool):
def build(components_directory: str, component_filepattern: str, engine: str,
kfp_package_path: Optional[str], overwrite_dockerfile: bool,
push_image: bool):
"""Builds containers for KFP v2 Python-based components."""
if engine != 'docker':
@ -374,6 +372,7 @@ def build(components_directory: pathlib.Path, component_filepattern: str,
stacklevel=2)
sys.exit(1)
components_directory = pathlib.Path(components_directory)
components_directory = components_directory.resolve()
if not components_directory.is_dir():
@ -389,6 +388,8 @@ def build(components_directory: pathlib.Path, component_filepattern: str,
' optional dependencies by running `pip install kfp[all]`.')
raise sys.exit(1)
kfp_package_path = pathlib.Path(
kfp_package_path) if kfp_package_path is not None else None
builder = ComponentBuilder(
context_directory=components_directory,
kfp_package_path=kfp_package_path,

99
sdk/python/kfp/cli/component_test.py
View File

@ -13,6 +13,7 @@
# limitations under the License.
"""Tests for `components` command group in KFP CLI."""
import contextlib
import os
import pathlib
import textwrap
import unittest
@ -83,17 +84,17 @@ class Test(unittest.TestCase):
return super().setUp()
def assertFileExists(self, path: str):
def assert_file_exists(self, path: str):
path_under_test_dir = self._working_dir / path
self.assertTrue(path_under_test_dir, f'File {path} does not exist!')
def assertFileExistsAndContains(self, path: str, expected_content: str):
self.assertFileExists(path)
def assert_file_exists_and_contains(self, path: str, expected_content: str):
self.assert_file_exists(path)
path_under_test_dir = self._working_dir / path
got_content = path_under_test_dir.read_text()
self.assertEqual(got_content, expected_content)
def testKFPConfigForSingleFile(self):
def test_kfp_config_for_single_file(self):
preprocess_component = _make_component(
func_name='preprocess', target_image='custom-image')
train_component = _make_component(
@ -107,7 +108,7 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains(
self.assert_file_exists_and_contains(
'kfp_config.ini',
textwrap.dedent('''\
[Components]
@ -116,7 +117,7 @@ class Test(unittest.TestCase):
'''))
def testKFPConfigForSingleFileUnderNestedDirectory(self):
def test_kfp_config_for_single_file_under_nested_directory(self):
preprocess_component = _make_component(
func_name='preprocess', target_image='custom-image')
train_component = _make_component(
@ -130,7 +131,7 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains(
self.assert_file_exists_and_contains(
'kfp_config.ini',
textwrap.dedent('''\
[Components]
@ -139,7 +140,7 @@ class Test(unittest.TestCase):
'''))
def testKFPConfigForMultipleFiles(self):
def test_kfp_config_for_multiple_files(self):
component = _make_component(
func_name='preprocess', target_image='custom-image')
_write_components('preprocess_component.py', component)
@ -154,7 +155,7 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains(
self.assert_file_exists_and_contains(
'kfp_config.ini',
textwrap.dedent('''\
[Components]
@ -163,7 +164,7 @@ class Test(unittest.TestCase):
'''))
def testKFPConfigForMultipleFilesUnderNestedDirectories(self):
def test_kfp_config_for_multiple_files_under_nested_directories(self):
component = _make_component(
func_name='preprocess', target_image='custom-image')
_write_components('preprocess/preprocess_component.py', component)
@ -178,7 +179,7 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains(
self.assert_file_exists_and_contains(
'kfp_config.ini',
textwrap.dedent('''\
[Components]
@ -187,7 +188,7 @@ class Test(unittest.TestCase):
'''))
def testTargetImageMustBeTheSameInAllComponents(self):
def test_target_image_must_be_the_same_in_all_components(self):
component_one = _make_component(func_name='one', target_image='image-1')
component_two = _make_component(func_name='two', target_image='image-1')
_write_components('one_two/one_two.py', [component_one, component_two])
@ -205,7 +206,8 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 1)
def testTargetImageMustBeTheSameInAllComponentsWithBaseImage(self):
def test_target_image_must_be_the_same_in_all_components_with_base_image(
self):
component_one = _make_component(
func_name='one', base_image='image-1', target_image='target-image')
component_two = _make_component(
@ -227,7 +229,7 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 1)
def testComponentFilepatternCanBeUsedToRestrictDiscovery(self):
def test_component_filepattern_can_be_used_to_restrict_discovery(self):
component = _make_component(
func_name='preprocess', target_image='custom-image')
_write_components('preprocess/preprocess_component.py', component)
@ -245,7 +247,7 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains(
self.assert_file_exists_and_contains(
'kfp_config.ini',
textwrap.dedent('''\
[Components]
@ -253,17 +255,17 @@ class Test(unittest.TestCase):
'''))
def testEmptyRequirementsTxtFileIsGenerated(self):
def test_empty_requirements_txt_file_is_generated(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
result = self.runner.invoke(self.cli, ['build', str(self._working_dir)])
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains('requirements.txt',
'# Generated by KFP.\n')
self.assert_file_exists_and_contains('requirements.txt',
'# Generated by KFP.\n')
def testExistingRequirementsTxtFileIsUnchanged(self):
def test_existing_requirements_txt_file_is_unchanged(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -272,17 +274,17 @@ class Test(unittest.TestCase):
result = self.runner.invoke(self.cli, ['build', str(self._working_dir)])
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains('requirements.txt',
'Some pre-existing content')
self.assert_file_exists_and_contains('requirements.txt',
'Some pre-existing content')
def testDockerignoreFileIsGenerated(self):
def test_docker_ignore_file_is_generated(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
result = self.runner.invoke(self.cli, ['build', str(self._working_dir)])
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains(
self.assert_file_exists_and_contains(
'.dockerignore',
textwrap.dedent('''\
# Generated by KFP.
@ -290,7 +292,7 @@ class Test(unittest.TestCase):
component_metadata/
'''))
def testExistingDockerignoreFileIsUnchanged(self):
def test_existing_docker_ignore_file_is_unchanged(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -299,10 +301,10 @@ class Test(unittest.TestCase):
result = self.runner.invoke(self.cli, ['build', str(self._working_dir)])
self.assertEqual(result.exit_code, 0)
self.assertFileExistsAndContains('.dockerignore',
'Some pre-existing content')
self.assert_file_exists_and_contains('.dockerignore',
'Some pre-existing content')
def testDockerEngineIsSupported(self):
def test_docker_engine_is_supported(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -315,7 +317,7 @@ class Test(unittest.TestCase):
self._docker_client.images.push.assert_called_once_with(
'custom-image', stream=True, decode=True)
def testKanikoEngineIsNotSupported(self):
def test_kaniko_engine_is_not_supported(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -329,7 +331,7 @@ class Test(unittest.TestCase):
self._docker_client.api.build.assert_not_called()
self._docker_client.images.push.assert_not_called()
def testCloudBuildEngineIsNotSupported(self):
def test_cloud_build_engine_is_not_supported(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -346,7 +348,7 @@ class Test(unittest.TestCase):
self._docker_client.api.build.assert_not_called()
self._docker_client.images.push.assert_not_called()
def testDockerClientIsCalledToBuildAndPushByDefault(self):
def test_docker_client_is_called_to_build_and_push_by_default(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -361,7 +363,7 @@ class Test(unittest.TestCase):
self._docker_client.images.push.assert_called_once_with(
'custom-image', stream=True, decode=True)
def testDockerClientIsCalledToBuildButSkipsPushing(self):
def test_docker_client_is_called_to_build_but_skips_pushing(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -376,7 +378,7 @@ class Test(unittest.TestCase):
self._docker_client.images.push.assert_not_called()
@mock.patch('kfp.__version__', '1.2.3')
def testDockerfileIsCreatedCorrectly(self):
def test_docker_file_is_created_correctly(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -387,7 +389,7 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 0)
self._docker_client.api.build.assert_called_once()
self.assertFileExistsAndContains(
self.assert_file_exists_and_contains(
'Dockerfile',
textwrap.dedent('''\
# Generated by KFP.
@ -402,7 +404,7 @@ class Test(unittest.TestCase):
COPY . .
'''))
def testExistingDockerfileIsUnchangedByDefault(self):
def test_existing_dockerfile_is_unchanged_by_default(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -414,11 +416,11 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 0)
self._docker_client.api.build.assert_called_once()
self.assertFileExistsAndContains('Dockerfile',
'Existing Dockerfile contents')
self.assert_file_exists_and_contains('Dockerfile',
'Existing Dockerfile contents')
@mock.patch('kfp.__version__', '1.2.3')
def testExistingDockerfileCanBeOverwritten(self):
def test_existing_dockerfile_can_be_overwritten(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
@ -430,7 +432,7 @@ class Test(unittest.TestCase):
)
self.assertEqual(result.exit_code, 0)
self._docker_client.api.build.assert_called_once()
self.assertFileExistsAndContains(
self.assert_file_exists_and_contains(
'Dockerfile',
textwrap.dedent('''\
# Generated by KFP.
@ -445,24 +447,26 @@ class Test(unittest.TestCase):
COPY . .
'''))
def testDockerfileCanContainCustomKFPPackage(self):
def test_dockerfile_can_contain_custom_kfp_package(self):
component = _make_component(
func_name='train', target_image='custom-image')
_write_components('components.py', component)
current_dir = os.path.dirname(os.path.abspath(__file__))
package_dir = os.path.dirname(os.path.dirname(current_dir))
result = self.runner.invoke(
self.cli,
[
'build',
str(self._working_dir),
'--kfp-package-path=/Some/localdir/containing/kfp/source'
str(self._working_dir), f'--kfp-package-path={package_dir}'
],
)
self.assertEqual(result.exit_code, 0)
self._docker_client.api.build.assert_called_once()
self.assertFileExistsAndContains(
'Dockerfile',
textwrap.dedent('''\
self.assert_file_exists('Dockerfile')
with open('Dockerfile', 'r') as f:
contents = f.read()
file_start = textwrap.dedent('''\
# Generated by KFP.
FROM python:3.7
@ -470,10 +474,9 @@ class Test(unittest.TestCase):
WORKDIR /usr/local/src/kfp/components
COPY requirements.txt requirements.txt
RUN pip install --no-cache-dir -r requirements.txt
RUN pip install --no-cache-dir /Some/localdir/containing/kfp/source
COPY . .
'''))
''')
self.assertTrue(contents.startswith(file_start))
self.assertRegex(contents, 'RUN pip install --no-cache-dir kfp-*')
if __name__ == '__main__':

12
sdk/python/kfp/cli/dsl_compile.py
View File

@ -89,10 +89,13 @@ class PipelineCollectorContext():
@click.command()
@click.option(
'--py', type=str, required=True, help='Local absolute path to a py file.')
'--py',
type=click.Path(exists=True, dir_okay=False),
required=True,
help='Local absolute path to a py file.')
@click.option(
'--output',
type=str,
type=click.Path(exists=False, dir_okay=False),
required=True,
help=parsing.get_param_descr(_compile_pipeline_function, 'package_path'))
@click.option(
@ -148,6 +151,11 @@ def main():
logging.basicConfig(format='%(message)s', level=logging.INFO)
try:
dsl_compile.help = '(Deprecated. Please use `kfp dsl compile` instead.)\n\n' + dsl_compile.help
logging.error(
'`dsl-compile` is deprecated. Please use `kfp dsl compile` instead.'
)
dsl_compile(obj={}, auto_envvar_prefix='KFP')
except Exception as e:
click.echo(str(e), err=True)

14
sdk/python/kfp/cli/experiment.py
View File

@ -29,9 +29,9 @@ def create(ctx: click.Context, description: str, name: str):
client = ctx.obj['client']
output_format = ctx.obj['output']
response = client.create_experiment(name, description=description)
_display_experiment(response, output_format)
click.echo('Experiment created.')
experiment = client.create_experiment(name, description=description)
_display_experiment(experiment, output_format)
click.echo(f'Created experiment {experiment.id}.')
@experiment.command()
@ -100,7 +100,7 @@ def delete(ctx: click.Context, experiment_id: str):
client = ctx.obj['client']
client.delete_experiment(experiment_id)
click.echo(f'Experiment {experiment_id} deleted.')
click.echo(f'Deleted experiment {experiment_id}.')
def _display_experiments(experiments: List[ApiExperiment],
@ -155,7 +155,7 @@ def archive(ctx: click.Context, experiment_id: str, experiment_name: str):
experiment_id = experiment.id
client.archive_experiment(experiment_id=experiment_id)
click.echo(f'Experiment {experiment_id} archived.')
click.echo(f'Archived experiment {experiment_id}.')
@experiment.command()
@ -182,5 +182,5 @@ def unarchive(ctx: click.Context, experiment_id: str, experiment_name: str):
experiment = client.get_experiment(experiment_name=experiment_name)
experiment_id = experiment.id
client.archive_experiment(experiment_id=experiment_id)
click.echo(f'Experiment {experiment_id} unarchived.')
client.unarchive_experiment(experiment_id=experiment_id)
click.echo(f'Unarchived experiment {experiment_id}.')

9
sdk/python/kfp/cli/pipeline.py
View File

@ -58,7 +58,7 @@ def create(ctx: click.Context,
pipeline = client.upload_pipeline(package_file, pipeline_name, description)
_display_pipeline(pipeline, output_format)
click.echo('Pipeline uploaded successfully.')
click.echo(f'Created pipeline {pipeline.id}.')
either_option_required = 'Either --pipeline-id or --pipeline-name is required.'
@ -85,7 +85,7 @@ either_option_required = 'Either --pipeline-id or --pipeline-name is required.'
'pipeline_version_name'),
required=True,
)
@click.argument('package-file')
@click.argument('package-file', type=click.Path(exists=True, dir_okay=False))
@click.pass_context
def create_version(ctx: click.Context,
package_file: str,
@ -106,6 +106,7 @@ def create_version(ctx: click.Context,
version = client.pipeline_uploads.upload_pipeline_version(
package_file, name=pipeline_version, pipelineid=pipeline_id)
_display_pipeline_version(version, output_format)
click.echo(f'Created pipeline version {version.id}.')
@pipeline.command()
@ -213,7 +214,7 @@ def delete_version(ctx: click.Context, version_id: str):
client = ctx.obj['client']
res = client.delete_pipeline_version(version_id)
click.echo(f'Pipeline version {version_id} deleted.')
click.echo(f'Deleted pipeline version {version_id}.')
return res
@ -253,7 +254,7 @@ def delete(ctx: click.Context, pipeline_id: str):
return
client.delete_pipeline(pipeline_id)
click.echo(f'{pipeline_id} deleted.')
click.echo(f'Deleted pipeline {pipeline_id}.')
def _print_pipelines(pipelines: List[kfp_server_api.ApiPipeline],

15
sdk/python/kfp/cli/recurring_run.py
View File

@ -50,6 +50,7 @@ either_option_required = 'Either --experiment-id or --experiment-name is require
@click.option(
'--enable-caching/--disable-caching',
type=bool,
default=None,
help=parsing.get_param_descr(client.Client.create_recurring_run,
'enable_caching'),
)
@ -81,6 +82,7 @@ either_option_required = 'Either --experiment-id or --experiment-name is require
@click.option(
'--max-concurrency',
type=int,
default=1,
help=parsing.get_param_descr(client.Client.create_recurring_run,
'max_concurrency'),
)
@ -91,6 +93,7 @@ either_option_required = 'Either --experiment-id or --experiment-name is require
)
@click.option(
'--pipeline-package-path',
type=click.Path(exists=True, dir_okay=False),
help=parsing.get_param_descr(client.Client.create_recurring_run,
'pipeline_package_path'))
@click.option(
@ -121,6 +124,17 @@ def create(ctx: click.Context,
client = ctx.obj['client']
output_format = ctx.obj['output']
if enable_caching is not None:
click.echo(
'--enable-caching and --disable-caching options are not yet supported.'
)
enable_caching = None
if (interval_second is None) == (cron_expression is None):
raise ValueError(
'Either of --interval-second or --cron-expression options is required.'
)
if (experiment_id is None) == (experiment_name is None):
raise ValueError(either_option_required)
if not experiment_id:
@ -148,6 +162,7 @@ def create(ctx: click.Context,
start_time=start_time,
version_id=version_id)
_display_recurring_run(recurring_run, output_format)
click.echo(f'Created job {recurring_run.id}.')
@recurring_run.command()

7
sdk/python/kfp/cli/run.py
View File

@ -150,6 +150,7 @@ def create(ctx: click.Context, experiment_name: str, run_name: str,
_wait_for_run_completion(client, run.id, timeout, output_format)
else:
_display_run(client, namespace, run.id, watch, output_format)
click.echo(f'Created run {run.id}.')
@run.command()
@ -192,7 +193,7 @@ def archive(ctx: click.Context, run_id: str):
sys.exit(1)
client.archive_run(run_id=run_id)
click.echo(f'{run_id} archived.')
click.echo(f'Archived run {run_id}.')
@run.command()
@ -211,7 +212,7 @@ def unarchive(ctx: click.Context, run_id: str):
sys.exit(1)
client.unarchive_run(run_id=run_id)
click.echo(f'{run_id} unarchived.')
click.echo(f'Unarchived run {run_id}.')
@run.command()
@ -227,7 +228,7 @@ def delete(ctx: click.Context, run_id: str):
client = ctx.obj['client']
client.delete_run(run_id)
click.echo(f'{run_id} deleted.')
click.echo(f'Deleted run {run_id}.')
def _display_run(client: client.Client,