Add TensorBoard Integration #15

wbuchwalter · 2017-08-06T01:21:21Z

Fix #13 .

Notes:

To add some consistency, I modify the existing spec to be camelCase everywhere instead of snake_case, all standard k8s specs already being in camelCase.
We discussed assuming that we don't need to expose the service outside the cluster. But since it was a really small change to add I added the option. By default TensorBoard's service is using ClusterIP, but you can change the ServiceType to be what you want. For example:

tensorboard:
    logDir: /tmp/tensorflow
    serviceType: LoadBalancer

Let me know if you are okay with the 2 points above!

Todos:

Base logic
Tests
Rebase master
Update documentation

jlewi

This is great. Thank you very much.

Do you think its worth writing a unittest for TBReplicaSet?

Do you think its worth updating the helm E2E tests to verify TensorBoard is created and deleted? I'd be ok with filing a PR for that and leaving it for the future.

jlewi · 2017-08-07T13:12:04Z

glide.yaml

@@ -10,7 +10,7 @@ import:
  version: 6d8c23d2e66ce61e2a7e34f92e8eb3eec13279fa
 - package: github.com/coreos/etcd
  version: v3.1.9
- package: github.com/Sirupsen/logrus
+- package: github.com/sirupsen/logrus


Do we actually use this package?
The original etcd operator used the Sirupsen package. I liked the idea of structured logging because I think we want to tag log entries so we know which worker they came from. But it seemed like that package didn't support logging filename and linenumber. So I settled on using the Google log package for now.

I'll remove it. Glog is perfectly fine so far.

Was this removed? (I'm still new to GitHub code review).

jlewi · 2017-08-07T13:13:10Z

pkg/trainer/tensorboard.go

+	"mlkube.io/pkg/spec"
+)
+
+//TBReplicaSet represent the RS for the TensorBoard instance


Nit space after //

jlewi · 2017-08-07T13:13:44Z

pkg/trainer/tensorboard.go

+}
+
+func (s *TBReplicaSet) Create() error {
+	//By default we assume TensorBoard's service will be a ClusterIP


nit space after //

jlewi · 2017-08-07T13:14:05Z

pkg/trainer/tensorboard.go

+		st = s.Spec.ServiceType
+	}
+
+	//create the service exposing TensorBoard


space after //

jlewi · 2017-08-07T13:17:56Z

pkg/trainer/tensorboard.go

+		failures = true
+	}
+
+	err = s.ClientSet.CoreV1().Pods(NAMESPACE).DeleteCollection(&meta_v1.DeleteOptions{}, listOpts)


Do we need to delete pods? Won't deleting the deployment take care of it?

jlewi · 2017-08-07T13:18:19Z

pkg/trainer/tensorboard.go

+		failures = true
+	}
+
+	err = s.ClientSet.ExtensionsV1beta1().ReplicaSets(NAMESPACE).DeleteCollection(&meta_v1.DeleteOptions{}, listOpts)


Do we need to delete the replica set? Won't deleting the deployment take care of it?

Not by default it seems. All my tests showed that this was not the case. Deleting the Deployment did not delete the RS, and deleting the RS did not delete the pods.
I'm guessing I need to look at the DeleteOptions and tune the PropagationPolicy for that to happen. I'll take a look.

jlewi · 2017-08-07T13:18:57Z

pkg/trainer/tensorboard.go

+		},
+		Ports: []v1.ContainerPort{
+			{
+				ContainerPort: 6006,


nit could we define a constant for 6006 since we use it in 2 different places.

jlewi · 2017-08-07T13:23:23Z

pkg/trainer/tensorboard.go

+func (s *TBReplicaSet) getDeploymentSpecTemplate() v1.PodTemplateSpec {
+	c := &v1.Container{
+		Name:  s.jobName(),
+		Image: "tensorflow/tensorflow",


Maybe add a TODO here to make the TensorFlow image a parameter of the job operator. I think that there's a couple places where we might want to just use the default TensorFlow image. For example in parameter servers we can just start the TensorFlow server.

However, users might want to specify which version of TensorFlow they want to use e.g. 1.0/1.2 etc...

Good point.
What about this being a parameter of the TfJob and not the a param of the operator?
For large teams that may want to share a single kubernetes cluster, different projects might be running different versions of TF for example. Also CNTK supports TensorBoard but maybe only specific versions etc.

I think we want the TfJob operator to specify a default and then allow it to be overwritten on a per job basis.

For overloading per job, I'm not sure whether we should have a specific TfJob field to specify a default TF image for the job that overwrites the default provided by the operator. Or if we should just allow e.g. subcomponent (e.g. TBReplicaSet, TfReplicaSet to specify a container image).

wbuchwalter · 2017-08-07T18:34:09Z

Do you think its worth writing a unittest for TBReplicaSet?

Do you think its worth updating the helm E2E tests to verify TensorBoard is created and deleted? I'd be ok with filing a PR for that and leaving it for the future.

Yes I think I should add some test. If you would like this to be merged early, I can do it in a separate PR, otherwise I'll do it here.

jlewi · 2017-08-07T20:43:31Z

Lets do the tests in this PR if you don't mind. Thank you.

wbuchwalter · 2017-08-14T15:56:34Z

@jlewi I implemented your suggestions, let me know what you think! Sorry for the delay, last week was pretty busy on my side.
Also I update Travis's configuration to install glide with apt as the build was failing with Sorry, we dont have a dist for your system: linux amd64. See Masterminds/glide#639.

FYI, i'll be travelling for the next two weeks with very limited internet access, so In case you require further changes feel free to update the PR if you don't want to wait.

jlewi

Thank you! I just had a quick question about the test. If I don't hear from you, I'll go ahead and merge it anyway.

jlewi · 2017-08-14T23:16:13Z

pkg/trainer/tensorboard_test.go

+	}
+
+	// Check that a service was created.
+	sList, err := clientSet.CoreV1().Services(NAMESPACE).List(meta_v1.ListOptions{})


Any particular reason you use list here and not get?

No, my bad I should change that.
I'm currently in the plane so I added a Todo as I won't have time to test. Since there should never be more than one service the test still work as expected, but I will change it later for improved clarity.

jlewi · 2017-08-14T23:17:23Z

glide.yaml

@@ -10,7 +10,7 @@ import:
  version: 6d8c23d2e66ce61e2a7e34f92e8eb3eec13279fa
 - package: github.com/coreos/etcd
  version: v3.1.9
- package: github.com/Sirupsen/logrus
+- package: github.com/sirupsen/logrus


Was this removed? (I'm still new to GitHub code review).

jlewi

LGTM thank you.

Signed-off-by: Syulin7 <735122171@qq.com>

Add TensorBoard

fe0a0cb

jlewi reviewed Aug 7, 2017

View reviewed changes

wbuchwalter added 3 commits August 14, 2017 11:03

Add tests for TensorBoard + review comments

6a3e76a

Merge branch 'master' into tensorboard

d8b614a

Update doc and example

ac6800d

wbuchwalter changed the title ~~[WIP] Add TensorBoard Integration~~ Add TensorBoard Integration Aug 14, 2017

fix glide issue on travis

7fe4983

jlewi reviewed Aug 14, 2017

View reviewed changes

remote Sirupsen package

79bfd21

jlewi approved these changes Aug 16, 2017

View reviewed changes

jlewi merged commit 392d54c into kubeflow:master Aug 16, 2017

Syulin7 added a commit to Syulin7/training-operator that referenced this pull request Jun 5, 2023

Fix: ttlSecondsAfterFinished not taking effect. (kubeflow#15)

b81dfd7

Signed-off-by: Syulin7 <735122171@qq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TensorBoard Integration #15

Add TensorBoard Integration #15

wbuchwalter commented Aug 6, 2017 •

edited

Loading

jlewi left a comment

jlewi Aug 7, 2017

wbuchwalter Aug 7, 2017

jlewi Aug 14, 2017

jlewi Aug 7, 2017

jlewi Aug 7, 2017

jlewi Aug 7, 2017

jlewi Aug 7, 2017

jlewi Aug 7, 2017

wbuchwalter Aug 7, 2017

jlewi Aug 7, 2017

jlewi Aug 7, 2017

wbuchwalter Aug 7, 2017

jlewi Aug 7, 2017

wbuchwalter commented Aug 7, 2017

jlewi commented Aug 7, 2017

wbuchwalter commented Aug 14, 2017

jlewi left a comment

jlewi Aug 14, 2017

wbuchwalter Aug 15, 2017 •

edited

Loading

jlewi Aug 14, 2017

jlewi left a comment

Add TensorBoard Integration #15

Add TensorBoard Integration #15

Conversation

wbuchwalter commented Aug 6, 2017 • edited Loading

jlewi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbuchwalter commented Aug 7, 2017

jlewi commented Aug 7, 2017

wbuchwalter commented Aug 14, 2017

jlewi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbuchwalter Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlewi left a comment

Choose a reason for hiding this comment

wbuchwalter commented Aug 6, 2017 •

edited

Loading

wbuchwalter Aug 15, 2017 •

edited

Loading