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

[pytorch_mnist] Automate image build #490

Merged
merged 49 commits into from Jun 14, 2019

Conversation

dsdinter
Copy link
Contributor

@dsdinter dsdinter commented Jan 27, 2019

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 Reviewable

@googlebot
Copy link

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 cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Contributor Author

@dsdinter dsdinter left a 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

pytorch_mnist/Makefile Outdated Show resolved Hide resolved
@dsdinter dsdinter force-pushed the pytorch-nmist-tests branch 2 times, most recently from 48f6877 to 7e24499 Compare February 17, 2019 12:27
@jlewi
Copy link
Contributor

jlewi commented Feb 19, 2019

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.

@dsdinter dsdinter changed the title [WIP] [pytorch_mnist] Implement E2E tests [pytorch_mnist] Implement E2E tests Feb 19, 2019
@jlewi jlewi removed the request for review from cwbeitel March 26, 2019 02:51
@jlewi
Copy link
Contributor

jlewi commented Mar 26, 2019

/assign @texasmichelle
/assign @johnugeorge

@johnugeorge Since you are one of the primary maintainers of PyTorch do you want to review this PR?

@johnugeorge
Copy link
Member

With the upcoming release 0.5, v1alpha2 won't be supported. Supported versions would be v1beta1 and v1beta2

@dsdinter
Copy link
Contributor Author

dsdinter commented May 1, 2019

Hi @johnugeorge , thanks for the feedback. I bumped up versions to support v0.5.
I would appreciate your help to figure out the best way to test train and deploy jobs for the pytorch model.
I believe the idea at the moment is to use the test_runner module from tf-operator but wondering if we can use something like pytest to reduce dependencies between components.

Thanks.

@jlewi
Copy link
Contributor

jlewi commented May 3, 2019

Yes. You can use the pytest module. I believe mnist TF is using tat now at least for deploy.
https://github.com/kubeflow/examples/blob/master/mnist/testing/predict_test.py

@johnugeorge @dsdinter what's the best way to push through this PR. It looks like this PR is doing three things

  1. Setup CI for the docker images.
  2. Adding E2E test for training
  3. Adding E2E test for prediction.

Would breaking this up into 3 smaller PRs help make progress?

@johnugeorge
Copy link
Member

I have the same opinion. @dsdinter what do you think?

@dsdinter
Copy link
Contributor Author

dsdinter commented May 3, 2019

I agree, part 1. is complete I believe.
Happy to change the test modules as dummy ones to pass always, then close this PR, let me change the title.

@dsdinter dsdinter changed the title [pytorch_mnist] Implement E2E tests [pytorch_mnist] Automate image build May 3, 2019
@jlewi
Copy link
Contributor

jlewi commented May 3, 2019

I'll let @johnugeorge handle reviewing this PR; if you need me for something let me know.

@jlewi jlewi removed their request for review May 3, 2019 23:01
Remove no longer used test_data and conftest
Remove no longer used test_data and conftest
@dsdinter
Copy link
Contributor Author

dsdinter commented May 6, 2019

Thanks @jlewi , @johnugeorge please review and let me know if you are happy to approve.

Kind regards.

@johnugeorge
Copy link
Member

Looks good to me. Anything pending in this?

@johnugeorge
Copy link
Member

/lgtm

@dsdinter
Copy link
Contributor Author

dsdinter commented May 8, 2019

/approve

@dsdinter
Copy link
Contributor Author

dsdinter commented May 8, 2019

@texasmichelle @jlewi feel free to approve this PR.

Thanks.

@texasmichelle
Copy link
Member

Thanks to you both for working on this!

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 7a6dc7b into kubeflow:master Jun 14, 2019
@dsdinter dsdinter deleted the pytorch-nmist-tests branch June 28, 2019 14:31
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.

[mnist_pytorch] Implement E2E tests
6 participants