-
Notifications
You must be signed in to change notification settings - Fork 756
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
[pytorch_mnist] Automate image build #490
Conversation
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
e6ca26b
to
8863deb
Compare
CLAs look good, thanks! |
8863deb
to
4f5e4ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will use GCP Cloud builder for Docker https://github.com/GoogleCloudPlatform/cloud-builders
48f6877
to
7e24499
Compare
Just an FYI; if you want me to review this remove WIP from the title. I assume if WIP is in the title its not ready for review. |
/assign @texasmichelle @johnugeorge Since you are one of the primary maintainers of PyTorch do you want to review this PR? |
With the upcoming release 0.5, v1alpha2 won't be supported. Supported versions would be v1beta1 and v1beta2 |
Keep postsubmit jobs as original release job to push images to examples registry
Rename testing package as kubeflow/training-operator#945 Added correct path to import test framework from tf-operator
…v1beta1/v1beta2 to support Kubeflow 0.5 - Refactor training manifests from v1alpha2 to v1beta2 - Update documents
12a4ccb
to
1fa7f80
Compare
Hi @johnugeorge , thanks for the feedback. I bumped up versions to support v0.5. Thanks. |
Yes. You can use the pytest module. I believe mnist TF is using tat now at least for deploy. @johnugeorge @dsdinter what's the best way to push through this PR. It looks like this PR is doing three things
Would breaking this up into 3 smaller PRs help make progress? |
I have the same opinion. @dsdinter what do you think? |
I agree, part 1. is complete I believe. |
I'll let @johnugeorge handle reviewing this PR; if you need me for something let me know. |
Remove no longer used test_data and conftest
Remove no longer used test_data and conftest
Thanks @jlewi , @johnugeorge please review and let me know if you are happy to approve. Kind regards. |
Looks good to me. Anything pending in this? |
/lgtm |
/approve |
@texasmichelle @jlewi feel free to approve this PR. Thanks. |
Thanks to you both for working on this! /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsdinter, texasmichelle 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 |
First step to fix #312
TODO
Add E2E test to train model
Add E2E test to deploy model
Add E2E test to predict digits.
This change is