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.

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

Enhance hard code for export dir in deploy component #823

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

jinchihe
Copy link
Member

@jinchihe jinchihe commented Feb 13, 2019

While using deployer component to create a TF-Serving service, hit a issue that could not find base path for servable. Checked the details, that's caused by the export directory is hard code to model_path/export/export as below in deploy.sh.

ks param set server modelPath "${MODEL_PATH}/export/export"

To let the deployer component to be reusable, we should enhance that because the model export path can be specified during tensorflow/estimator training from the offical API.

For the TFX taxi sample, the export path is specified model_path/export/export in training step, so update taxi-cab-classification-pipeline.py as well. Thanks.


This change is Reviewable

@jinchihe
Copy link
Member Author

/assign @gaoning777

Could you please review the ticket? Thanks.

@gaoning777
Copy link
Contributor

Another place that needs modification is the notebook sample: https://github.com/kubeflow/pipelines/blob/master/samples/notebooks/KubeFlow%20Pipeline%20Using%20TFX%20OSS%20Components.ipynb.
Please update the deployer component in this sample, too.

@jinchihe
Copy link
Member Author

Another place that needs modification is the notebook sample: https://github.com/kubeflow/pipelines/blob/master/samples/notebooks/KubeFlow%20Pipeline%20Using%20TFX%20OSS%20Components.ipynb.
Please update the deployer component in this sample, too.

Addressed you comments. Great thanks for point out the missing. @gaoning777

@gaoning777
Copy link
Contributor

/test kubeflow-pipeline-sample-test

@jinchihe
Copy link
Member Author

@gaoning777 Any more comments? Thanks for reviewing.

@gaoning777
Copy link
Contributor

gaoning777 commented Feb 20, 2019

I saw the sample test failure. Do you mind taking a look at this?
The sample test is not explicitly shown in the github but we enabled them in the background.

https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/kubeflow_pipelines/823/kubeflow-pipeline-sample-test/736/

@gaoning777
Copy link
Contributor

gaoning777 commented Feb 20, 2019

Never mind. Since the new deployer component has not been built, thus sample failures with --model-export-path not found. This PR will lead to samples failures.

Could we separate this PR into two:

  1. deployer code change.
  2. sample change.
    When this first PR is merged, I will release the component images.
    In the second PR, I can also help update the image tags in the samples.
    This way, we will not have broken samples during any time.

jinchihe added a commit to jinchihe/pipelines that referenced this pull request Feb 21, 2019
@jinchihe
Copy link
Member Author

Never mind. Since the new deployer component has not been built, thus sample failures with --model-export-path not found. This PR will lead to samples failures.

Could we separate this PR into two:

  1. deployer code change.
  2. sample change.
    When this first PR is merged, I will release the component images.
    In the second PR, I can also help update the image tags in the samples.
    This way, we will not have broken samples during any time.

@gaoning777 Thanks for your comments!
I updated the PR to change the deploy script, and created new one #843 to update the sample TFX taxi.

@gaoning777
Copy link
Contributor

/lgtm

@gaoning777
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaoning777

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

@k8s-ci-robot k8s-ci-robot merged commit 1045e02 into kubeflow:master Feb 21, 2019
cheyang pushed a commit to alibaba/pipelines that referenced this pull request Mar 28, 2019
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants