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

feat(components): PyTorch - Added the Create PyTorch Model Archive component #5630

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented May 11, 2021

No description provided.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jun 3, 2021

Can someone please take a look?

@amygdala
Copy link
Contributor

amygdala commented Jun 3, 2021

sorry, somehow I missed this during a busy period. I will take a look tomorrow.

@zijianjoy
Copy link
Collaborator

/lgtm
/approve

/hold
Thank you Alexey! Feel free to unhold once you address the comments in the PR.

@Bobgy
Copy link
Contributor

Bobgy commented Jun 7, 2021

How is this component different from #5780?
Feels like we should only keep one choice.

/cc @chauhang

@google-oss-robot
Copy link

@Bobgy: GitHub didn't allow me to request PR reviews from the following users: chauhang.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

How is this component different from #5780?
Feels like we should only keep one choice.

/cc @chauhang

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.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jun 7, 2021

How is this component different from #5780?
Feels like we should only keep one choice.

/cc @chauhang

This PR has been pending the review for almost a month.
What was the main motivation to merge #5780, but not this PR?
This PR has a component that fits with the previously existing PyTorch components. This PR has a sample pipeline proving that the component work. There are future PRs that will use the output of this component to continue the pipleine.

Speaking about #5780, I do not see any KFP component there. I also do not see any KFP sample pipeline that uses a component.

I feel that anyone should be free to create their components, so I'm fine with the existence of (even though I do not fully understand its purpose). However I feel that this should be reciprocal - I do not think that some later PR (#5780) should block this PR.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jun 7, 2021

/unhold
/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jun 7, 2021

/test kubeflow-pipelines-samples-v2

1 similar comment
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jun 7, 2021

/test kubeflow-pipelines-samples-v2

@google-oss-robot
Copy link

@Ark-kun: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
kubeflow-pipelines-samples-v2 73694e5 link /test kubeflow-pipelines-samples-v2

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.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jun 8, 2021

/test kubeflow-pipelines-samples-v2

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Jun 8, 2021

The kubeflow-pipelines-samples-v2 tests actually succeed. So I'm merging this PR.

@Ark-kun Ark-kun merged commit b08b29f into kubeflow:master Jun 8, 2021
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.

6 participants