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

Jump to bottom

MLEngine Commands Implementation #773

Merged
merged 7 commits into from
Feb 16, 2019
Merged

MLEngine Commands Implementation #773

merged 7 commits into from
Feb 16, 2019

Conversation

hongye-sun
Copy link
Contributor

@hongye-sun hongye-sun commented Feb 2, 2019 •
edited by IronPan
Loading

The commands will be part of component sdk so that user can also directly use the APIs in their own code.

It includes APIs:

  • create_job
  • create_model
  • create_version
  • delete_version

This change is Reviewable

@hongye-sun hongye-sun changed the title [WIP] MLEngine Commands Implementation MLEngine Commands Implementation Feb 5, 2019
@vicaire
Copy link
Contributor

vicaire commented Feb 6, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vicaire

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hongye-sun
Copy link
Contributor Author

/retest

gaoning777
gaoning777 reviewed Feb 15, 2019
View reviewed changes
component_sdk/python/kfp_component/core/_display.py
html = obj._repr_html_()
_output_ui_metadata({
'type': 'web-app',
'html': html
Copy link
Contributor

Choose a reason for hiding this comment

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

In the documentation(https://www.kubeflow.org/docs/pipelines/output-viewer/), I did not find the html field key. Is the documentation outdated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new feature we will add. It's tracked by #795. @rileyjbauer

gaoning777
gaoning777 reviewed Feb 15, 2019
View reviewed changes
component_sdk/python/kfp_component/google/ml_engine/_client.py
import googleapiclient.discovery as discovery
from googleapiclient import errors

class MLEngineClient:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a TODO list in this client python about what other interfaces will be surfaced.
For example, model version list as in https://cloud.google.com/ml-engine/reference/rest/

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 added a module level comment for that.

@gaoning777
Copy link
Contributor

Overall, it looks good to me.
maybe later, we need to add some integration tests for these OOB components.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 16, 2019 •
edited
Loading

@hongye-sun: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
kubeflow-pipeline-build-image 292978f link /test kubeflow-pipeline-build-image
kubeflow-pipeline-sample-test 57b7122 link /test kubeflow-pipeline-sample-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@gaoning777
Copy link
Contributor

Discussed with Hongye offline:

  1. metadata are dumped to a temporary folder and until the artifact story is complete, we can expose the metadata to the frontend to render.
  2. common_ops do not set timeout, however, users can set timeout in the DSL since argo already supports it.
  3. the argo pod name collision is rare, plus the components also check the configuration to make sure the second job is a retry.

@gaoning777
Copy link
Contributor

/lgtm

@hongye-sun
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit bd1e770 into kubeflow:master Feb 16, 2019
cheyang pushed a commit to alibaba/pipelines that referenced this pull request Mar 28, 2019
@hongye-sun @k8s-ci-robot
MLEngine Commands Implementation (kubeflow#773)
2f70f2e 
* MLEngine API commands

* Add job_id_prefix to create_job command

* add create training job command

* add display API

* Refactor CMLE APIs

* Fix PR comments.

* Add warnings before overwrite an existing file.
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
@ckadner
Add performance test script (kubeflow#773)
121c40f 
* Add performance test script

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

* Enable script in single-user mode, unauthenticated

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

* Use explicit names for duplicate pipeline function names

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

* Run performance tests in parallel

Pipeline compilation still runs in sequence due to
compiler implementation limitations.

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

* Improve readability of console output

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

* Use decorator to track method execution times

This allows accurately recording compile times when running tests
in parallel, despite the restriction that pipeline compilation is
synchronized.

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>

* Assert presence of 'pipeline_name' keyword argument

Instead of raising a ValueError in '@time_it' decorator

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaoning777 gaoning777 gaoning777 left review comments

@IronPan IronPan Awaiting requested review from IronPan

@vicaire vicaire Awaiting requested review from vicaire

@Ark-kun Ark-kun Awaiting requested review from Ark-kun

Assignees

@gaoning777 gaoning777

@vicaire vicaire

Labels
approved lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hongye-sun @vicaire @k8s-ci-robot @gaoning777