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

Samples - Switched the build_component sample to the new container API #2279

Merged
merged 3 commits into from
Oct 5, 2019
Merged

Samples - Switched the build_component sample to the new container API #2279

merged 3 commits into from
Oct 5, 2019

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Oct 1, 2019 •
edited by jlewi
Loading

This change is Reviewable

@Ark-kun Ark-kun force-pushed the Samples---Switched-the-build_component-sample-to-the-new-container-API branch from 8e0f70c to 348c42d Compare October 1, 2019 18:35
@Ark-kun Ark-kun assigned SinaChavoshi and unassigned SinaChavoshi Oct 2, 2019
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Oct 4, 2019

@hongye-sun Do you have any objection to this change? It removes quite a lot of boilerplate and the users no longer needs to learn complicated concepts like compilation.

@gaoning777
Copy link
Contributor

what is the plan for the component build APIs? will build_python_component be deprecated? If not, could you create another sample to demonstrate the build_image_from_working_dir. If so, lgtm.

@numerology
Copy link

/lgtm
You might want to wait for hearing back from @hongye-sun

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Oct 4, 2019

what is the plan for the component build APIs? will build_python_component be deprecated?

I did not initiate this deprecation. AFAIK, @SinaChavoshi has requested for the component creation APIs to be reduced to one. Same with container-building APIs. Based on this request our team had two design reviews where we ultimately agreed to merge the build_python_component and func_to_container_op and also make it easy to automatically build container from working directory when components are being created (designed by @hongye-sun and me).

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Oct 4, 2019

You might want to wait for hearing back from @hongye-sun

Yes, I'll be waiting for @hongye-sun

@SinaChavoshi
Copy link
Contributor

SinaChavoshi commented Oct 4, 2019 •
edited
Loading

what is the plan for the component build APIs? will build_python_component be deprecated?

Just to confirm I strongly recommend reducing the number of ways that users can build components / containers. I think we should choose one way and deprecate the rest.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 4, 2019
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Oct 4, 2019

Talked with @hongye-sun offline.
He suggested:

  • Pre-create the requirements.txt file instead of creating it dynamically in a temporary directory.
  • Showcase setting the default_base_image_or_builder to build_image_from_working_dir so that it's used every time the user creates component with func_to_container_op.

I've implemented both of those suggestions (and also added the customized builder scenario).

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Oct 4, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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

@SinaChavoshi
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 3a868a9 into kubeflow:master Oct 5, 2019
@Ark-kun Ark-kun deleted the Samples---Switched-the-build_component-sample-to-the-new-container-API branch October 5, 2019 01:26
@Ark-kun Ark-kun mentioned this pull request Oct 12, 2019
Closed
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
@Iamlovingit
fix: hpa scale up failed, when using rawdeployment (kubeflow#2279)
21d1e66 
* fix: hpa scale up failed, when using rawdeployment

Signed-off-by: iamlovingit <bitfrog@163.com>

* use kmp.SafeDiff instead of equality.Semantic.DeepEqual

Signed-off-by: iamlovingit <bitfrog@163.com>

* remove semanticDeploymentEquals function for simplifing the code

Signed-off-by: iamlovingit <bitfrog@163.com>

* add handle err

Signed-off-by: iamlovingit <bitfrog@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SinaChavoshi SinaChavoshi SinaChavoshi left review comments

@hongye-sun hongye-sun Awaiting requested review from hongye-sun

@numerology numerology Awaiting requested review from numerology

@gaoning777 gaoning777 Awaiting requested review from gaoning777

Assignees

@gaoning777 gaoning777

@numerology numerology

@SinaChavoshi SinaChavoshi

@hongye-sun hongye-sun

Labels
approved area/samples lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Ark-kun @gaoning777 @numerology @SinaChavoshi @k8s-ci-robot @hongye-sun