Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to upload a version of the pipeline with CLI #3672

Merged
merged 6 commits into from
May 7, 2020
Merged

Make it possible to upload a version of the pipeline with CLI #3672

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c7afc8
Add upload_pipeline_version to kfp.Client
shotarok May 1, 2020
70d7d44
Add the 'kfp pipeline upload_version' command to CLI
shotarok May 1, 2020
b9f51fc
Make sure to use upload_pipeline_version wihtout a helper func
shotarok May 5, 2020
be7ed6c
Make sure of the google coding style with yapf
shotarok May 5, 2020
55c8551
Fix up the pipeline id option
shotarok May 5, 2020
7ed5434
Make the version and id options to the required options
shotarok May 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions sdk/python/kfp/_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,18 @@ def upload_pipeline(self, pipeline_package_path, pipeline_name=None):
IPython.display.display(IPython.display.HTML(html))
return response

def upload_pipeline_version(self, pipeline_package_path, version_name, pipeline_id):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use client.pipeline_uploads.upload_pipeline_version() instead of creating a helper?

"""Uploads a version of the pipeline to the Kubeflow pipeline cluster.
Args:
pipeline_package_path: Local path to the pipeline package.
version_name: Version name of the pipeline.
pipeline_id: The string ID of the pipeline.
Returns:
A response object including details of a pipeline.
"""
response = self._upload_api.upload_pipeline_version(pipeline_package_path, name=version_name, pipelineid=pipeline_id)
return response

def get_pipeline(self, pipeline_id):
"""Get pipeline details.
Args:
Expand Down
25 changes: 25 additions & 0 deletions sdk/python/kfp/cli/pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,20 @@ def upload(ctx, pipeline_name, package_file):
_display_pipeline(pipeline)


@pipeline.command()
@click.argument("package-file")
@click.argument("pipeline-version")
@click.argument("pipeline-id")
Copy link
Member

@elikatsis elikatsis May 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have (at least some of them as) options instead? It is error-prone to require a specific order of numerous args.

It could be:

@click.option(
    "--id",
    "--pipeline-id",
    help="ID of the pipeline"
)
@click.option(
    "-v",
    "--pipeline-version",
    help="Name of the pipeline version"
)
@click.argument("package-file")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking they are the necessary arguments of client.pipeline_uploads.upload_pipeline_version when to implement it. I'll make them options. Thanks.

Copy link
Contributor Author

@shotarok shotarok May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elikatsis I changed them to the options, but I feel they should be arguments because of the following reasons.

  • When to run client.pipeline_uploads.upload_pipeline_version without a pipeline id, the API will return a 500 error
  • When to run client.pipeline_uploads.upload_pipeline_version without a version name, the API will use "None" as the version

Here is the diff to make the arguments options

code diff 
diff --git a/sdk/python/kfp/_client.py b/sdk/python/kfp/_client.py
index 0108314c..34fc25df 100644
--- a/sdk/python/kfp/_client.py
+++ b/sdk/python/kfp/_client.py
@@ -688,18 +688,6 @@ class Client(object):
       IPython.display.display(IPython.display.HTML(html))
     return response
 
-  def upload_pipeline_version(self, pipeline_package_path, version_name, pipeline_id):
-    """Uploads a version of the pipeline to the Kubeflow pipeline cluster.
-    Args:
-      pipeline_package_path: Local path to the pipeline package.
-      version_name: Version name of the pipeline.
-      pipeline_id: The string ID of the pipeline.
-    Returns:
-      A response object including details of a pipeline.
-    """
-    response = self._upload_api.upload_pipeline_version(pipeline_package_path, name=version_name, pipelineid=pipeline_id)
-    return response
-
   def get_pipeline(self, pipeline_id):
     """Get pipeline details.
     Args:
diff --git a/sdk/python/kfp/cli/pipeline.py b/sdk/python/kfp/cli/pipeline.py
index 9abe9a4b..930a5f29 100644
--- a/sdk/python/kfp/cli/pipeline.py
+++ b/sdk/python/kfp/cli/pipeline.py
@@ -43,16 +43,27 @@ def upload(ctx, pipeline_name, package_file):
 
 
 @pipeline.command()
+@click.option(
+    "-p",
+    "--pipeline-id",
+    help="ID of the pipeline"
+)
+@click.option(
+    "-v",
+    "--pipeline-version",
+    help="Name of the pipeline version"
+)
 @click.argument("package-file")
-@click.argument("pipeline-version")
-@click.argument("pipeline-id")
 @click.pass_context
 def upload_version(ctx, package_file, pipeline_version, pipeline_id):
     """Upload a version of the KFP pipeline"""
     client = ctx.obj["client"]
 
-    version = client.upload_pipeline_version(package_file, pipeline_version, pipeline_id)
-    logging.info("The {} version of the pipeline {} has been submitted\n".format(pipeline_version, pipeline_id))
+    version = client.pipeline_uploads.upload_pipeline_version(
+        package_file, name=pipeline_version, pipelineid=pipeline_id)
+    logging.info(
+        "The {} version of the pipeline {} has been submitted\n".format(
+            pipeline_version, pipeline_id))
     _display_pipeline_version(version)

Could you tell me your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you could set the click.option argument: required=True.
The following should work:

@click.option(
    "-v",
    "--pipeline-version",
    required=True,
    help="Name of the pipeline version"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the code in 70d7d44...7ed5434. PTAL.

@click.pass_context
def upload_version(ctx, package_file, pipeline_version, pipeline_id):
"""Upload a version of the KFP pipeline"""
client = ctx.obj["client"]

version = client.upload_pipeline_version(package_file, pipeline_version, pipeline_id)
logging.info("The {} version of the pipeline {} has been submitted\n".format(pipeline_version, pipeline_id))
_display_pipeline_version(version)


@pipeline.command()
@click.option(
"--max-size",
Expand Down Expand Up @@ -110,3 +124,14 @@ def _display_pipeline(pipeline):
headers = ["Parameter Name", "Default Value"]
data = [[param.name, param.value] for param in pipeline.parameters]
print(tabulate(data, headers=headers, tablefmt="grid"))


def _display_pipeline_version(version):
print(tabulate([], headers=["Pipeline Version Details"]))
pipeline_id = version.resource_references[0].key.id
table = [
["Pipeline ID", pipeline_id],
["Version Name", version.name],
["Uploaded at", version.created_at.isoformat()],
]
print(tabulate(table, tablefmt="plain"))