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

Create ks prototype for tf-batch-predict #1351

Merged
merged 9 commits into from Aug 17, 2018
Merged

Create ks prototype for tf-batch-predict #1351

merged 9 commits into from Aug 17, 2018

Conversation

yixinshi
Copy link
Member

@yixinshi yixinshi commented Aug 10, 2018

This change is Reviewable

@yixinshi
Copy link
Member Author

/assign jlewi

@yixinshi
Copy link
Member Author

/assign lluunn

@jlewi
Copy link
Contributor

jlewi commented Aug 13, 2018

I think we'd just like to have a simple prototype for the k8s/job to run the Dataflow job.
The prototype should go in the examples package.
You can take a look at the example here
https://github.com/kubeflow/kubeflow/blob/master/kubeflow/examples/prototypes/tf-job-simple.jsonnet

We want to put all the logic in the jsonnet file so that when the user runs ks generate they get the full spec and can easily customize it.

Copy link
Member Author

@yixinshi yixinshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @kunmingg and @richardsliu)

},
}, // tfBatchPredictContainerBase

tfBatchPredictContainer+: base.parts.tfBatchPredictContainerBase +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets simplify the prototype further. There's not much reason to pull out tfBatchPredictContainer and then override by merging in a dictionary; just paste the relevant dictionary describing the container into the container arrays below line 111.

We want the examples to be optimized for readability. The idea is that if people want to modify them (e.g. they want to set appropriate keys for AWS they would delete and replace the appropriate lines.

In other words, think of this as an "example" that people will copy and modify.

Ideally the example should run under some suitable set of conditions; e.g. assuming there is a valid GCP credential either as a VM service account or as a secret key if secretName is set.

I think we should keep conditionals for setting the following based on parameters

  1. num GPUs
  2. GCP secret.

@jlewi
Copy link
Contributor

jlewi commented Aug 17, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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 a63c927 into kubeflow:master Aug 17, 2018
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
* adding tf-batch-predict prototype

* first version of ks prototype

* change Readme

* more clean up

* adding an tf-batch-prediction jsonnet file into example

* an almost working version

* working version

* remove the code in kubeflow/kubeflow/tf-batch-predict

* simplify the jsonnet by removing unneeded nested structure.
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.

None yet

5 participants