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

Make TFJob operator a standalone prototype #743

Merged
merged 1 commit into from May 16, 2018

Conversation

pdmack
Copy link
Member

@pdmack pdmack commented Apr 30, 2018

Closes #669

PTAL
/cc @jlewi
/cc @DjangoPeng

Wasn't quite sure what to do with testing/workflows so left that more or less intact for now.


This change is Reviewable

@pdmack
Copy link
Member Author

pdmack commented Apr 30, 2018

ks init my-tf-job
cd my-tf-job/
ks registry add kubeflow github.com/pdmack/kubeflow/tree/669-tfjob-prototype/kubeflow
ks pkg install kubeflow/tf-job@669-tfjob-prototype
ks generate tf-job-operator tf-job-operator
ks apply default -c tf-job-operator

@pdmack pdmack changed the title Convert TFJob operator from core to its own prototype Convert TFJob operator from core to a tf-job prototype Apr 30, 2018
Copy link
Contributor

@jlewi jlewi left a comment

Choose a reason for hiding this comment

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

Sorry for the slow reply; thanks for the PR.

local spartakus = import "kubeflow/core/spartakus.libsonnet",
local centraldashboard = import "kubeflow/core/centraldashboard.libsonnet",
local version = import "kubeflow/core/version.libsonnet",

all:: jupyterhub.all(params)
+ tfjob.all(params)
+ ambassador.all(params)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the all prototype should continue to install the TFJob operator. This way folks who want all the components can install them with a single package. But we can also have a prototype for just TFJob.

@@ -0,0 +1,26 @@
// @apiVersion 0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think moving TFJob operator into the TFJob package makes sense? I'm uncertain what the proper organization of packages should be (see #106).

Its convenient for users not to install additional packages. Also, I think we'd like the all prototype to still install the TFJob operator. ksonnet doesn't handle dependencies between packages very well.

I'm not sure it makes sense to have a package just for TFJob. It might make sense to have a package for TensorFlow and put all the components for TensorFlow into that package.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. Seems I was a little overzealous in breaking out the TFJob operator. ;-)
I do still think it's important to distinguish it from the "user" TFJob prototype.
So, I would prefer to name it one of:

  • tf-job-operator
  • tf-job-admin
  • tf-job-service
  • etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

tf-job-operator seems like a good name to me for the prototype.

@pdmack pdmack changed the title Convert TFJob operator from core to a tf-job prototype Make TFJob operator a standalone prototype May 9, 2018
@pdmack pdmack changed the title Make TFJob operator a standalone prototype [WIP] Make TFJob operator a standalone prototype May 10, 2018
@jlewi
Copy link
Contributor

jlewi commented May 15, 2018

How's this coming?

@pdmack
Copy link
Member Author

pdmack commented May 15, 2018

@jlewi next up after /home/jovyan...back in earnest probably tomorrow.

@pdmack pdmack force-pushed the 669-tfjob-prototype branch 2 times, most recently from e42ffc2 to 32e0a4e Compare May 15, 2018 18:55
@pdmack
Copy link
Member Author

pdmack commented May 15, 2018

/retest

2 similar comments
@pdmack
Copy link
Member Author

pdmack commented May 15, 2018

/retest

@pdmack
Copy link
Member Author

pdmack commented May 15, 2018

/retest

@pdmack pdmack changed the title [WIP] Make TFJob operator a standalone prototype Make TFJob operator a standalone prototype May 15, 2018
@jlewi
Copy link
Contributor

jlewi commented May 16, 2018

Thanks!

/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 22678ae into kubeflow:master May 16, 2018
@pdmack pdmack deleted the 669-tfjob-prototype branch May 23, 2018 18:16
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
yanniszark pushed a commit to arrikto/kubeflow that referenced this pull request Feb 15, 2021
* feat: Fix the API

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Code generate

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Remove the implementation

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Fix CI

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Fix import

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Fix test case

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Fix test

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Remove manager test

Signed-off-by: Ce Gao <gaoce@caicloud.io>
surajkota pushed a commit to surajkota/kubeflow that referenced this pull request Jun 13, 2022
* Rename katib-manager to katib-db-manager

* update tags

* add files

* change mysql

* add missing files

* fix component name

* fix component name

* rename component name
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

3 participants