Support choosing tensorboard version from UI #2690

dldaisy · 2019-12-03T08:25:23Z

We allow choosing tensorboard version from UI. User now can select a tensorflow image version to start different versions of tensorboard among our provided list of images which can be opened correctly, including support for tensorboard 2.0.0+ by adding the argument "--bind-all". We also add a corresponding test to test version selection.

Changes include:
Frontend:
(1) Add html element in Tensorboard component to allow user to select the tensorboard version and start it.
(2) Modify functions about start/get a tensorboard in frontend server and apis. The functions now accept another arguments indicating tensorflow image version, encode it in url and request frontend server. The server will parse & decode the version and let k8s client to launch a required version of tensorboard. We don't encode version into viewer's name, so it just remains the same as before.

Backend:
We use the backend mentioned in PR #2544 with a field for tensorflow image. The backend viewer controller would use this field to launch a required version of tensorboard.

Note:
(1) Which versions to be allowed to choose: we tested all the main versions of tensorflow docker image, and kept all those can work correctly.
(2) Whether to change the viewer name with encoded information about version: Currently user can launch only one tensorboard viewer instance on one log directory, so we choose not to include the version information to identify a viewer's name. However, both solutions are correct and would be easy to modify later on.

Related bug: #2514

This change is

googlebot · 2019-12-03T08:25:33Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

k8s-ci-robot · 2019-12-03T08:25:38Z

Hi @dldaisy. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Bobgy · 2019-12-03T10:13:04Z

/ok-to-test

Bobgy

Thanks! this is great.

Can you help me format code by npm run format. That should reduce most diffs.

Bobgy · 2019-12-03T10:16:08Z

backend/src/crd/controller/viewer/reconciler/reconciler.go

@@ -180,6 +181,12 @@ func setPodSpecForTensorboard(view *viewerV1beta1.Viewer, s *corev1.PodSpec) {
 		// when https://github.com/kubeflow/pipelines/issues/2514 is done
 		// "--bind_all",
 	}
+
+	matched, _ := regexp.MatchString(`tensorflow/tensorflow:2.`, view.Spec.TensorboardSpec.TensorflowImage)


nit: you are not using any regex features, just using string.startsWith is simpler

Thanks for the review and suggestion! I will re-format the code today.

Bobgy · 2019-12-04T09:10:48Z

frontend/src/components/viewers/Tensorboard.test.tsx

@@ -19,7 +19,8 @@ import TensorboardViewer from './Tensorboard';
 import TestUtils from '../../TestUtils';
 import { Apis } from '../../lib/Apis';
 import { PlotType } from './Viewer';
-import { shallow } from 'enzyme';
+import { shallow, mount } from 'enzyme';
+import { resolve } from 'dns';


is this a mistake?

I will look into it. It might be me forgetting to delete something after writing test.

Bobgy · 2019-12-04T10:00:33Z

frontend/src/components/viewers/Tensorboard.tsx

@@ -56,6 +64,10 @@ class TensorboardViewer extends Viewer<TensorboardViewerProps, TensorboardViewer
    this._checkTensorboardApp();
  }

+  public onChangeFunc(e: React.ChangeEvent<{ name?: string; value: unknown }>): void {


2 notes:

recommend using arrow function as field like

public onChangeFunc = (e: ...) => { ... }

Arrow functions are bound automatically, then you can omit the .bind.. part when using it.

name the function by what it does makes this more readable. e.g. selectVersion or handleVersionSelect

I will rename the function. As for the arrow function, I didn't use it because when writing tests, I failed to invoke onChange arrow function with enzyme's simulate('change') method, although it works fine in real environment. Maybe we can discuss it later.

frontend/src/components/viewers/Tensorboard.tsx

Bobgy · 2019-12-04T10:15:46Z

frontend/src/components/viewers/Tensorboard.tsx

@@ -56,6 +64,10 @@ class TensorboardViewer extends Viewer<TensorboardViewerProps, TensorboardViewer
    this._checkTensorboardApp();
  }

+  public onChangeFunc(e: React.ChangeEvent<{ name?: string; value: unknown }>): void {


Behavior issue: after a version is selected, BusyButton state is no longer up-to-date, because it checks if the selected version instance exists or not.

I think the behavior could be confusing.
My imagined ideal behavior:

backend finds tensorboard instance by logdir and reports which version it is running

frontend shows to user there is an existing tensorboard with version xxx

user can choose to stop it and start a new one with different version

What do you think? Feel free to say your opinion. We discuss to make the product.

Yes, I agree. Since stopping and restarting a tensorboard requires a delete button, we plan to add the delete feature in next commit and the behavior will be completed as you mentioned above at that time.

Bobgy · 2019-12-04T10:28:24Z

frontend/src/components/viewers/Tensorboard.test.tsx

@@ -101,4 +108,35 @@ describe('Tensorboard', () => {
  it('is aggregatable', () => {
    expect(TensorboardViewer.prototype.isAggregatable()).toBeTruthy();
  });
+
+  it('set state', async () => {


Recommend reading this: https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change
Main point: test UI behavior, not implementation details.
Although the article advertises react testing library, but you can do similar testing style with enzyme.

Steps for this test:

mount the component (not shallow)

simulate clicking the select input

simulate clicking an item

verify UI shows your chosen item

click start tensorboard button, verify it called api with the correct values

In this way, no matter how you refactor this UI component, you test remains the same as long as user behavior is the same.

(please address behavior comments first, before you actually work on the test)

Bobgy · 2019-12-04T10:30:32Z

frontend/server/k8s-helper.ts

  if (!k8sV1CustomObjectClient) {
    throw new Error('Cannot access kubernetes Custom Object API');
  }
-  const currentPod = await getTensorboardInstance(logdir);
+  const currentPod = await getTensorboardInstance(logdir, tfversion);


What if there's a current pod with a different version?
Your steps below would fail because you are creating another viewer with the same name.

After some discussion with Jing, we find we are facing two choices:

Allow user to run more than one version at the same time, so with need to distinguish pods name by version. To implement this way, there might be several tensorboard deployments running at background, and the frontend will display which one will be opened.

Another choice is only allow one version of tensorboard running at the same time. Because users mainly hope to choose a version that can be used correctly, this implementation might be functionally enough while keep the page concise. To implement this way, the "logdir" would be an unique identifier of the tensorboard folder. It's not until the user delete the previous version that he can open a new version, within the same logdir.

Maybe we can discuss later to decide which one to choose!

2 sgtm
Do you have a preference?

dldaisy · 2019-12-24T02:23:36Z

/retest

Bobgy · 2019-12-24T03:13:36Z

frontend/server/package-lock.json

@@ -1769,7 +1769,8 @@
        },
        "ansi-regex": {
          "version": "2.1.1",
-          "bundled": true
+          "bundled": true,


Note, this still needs to be reverted.

frontend/src/components/viewers/Tensorboard.test.tsx

frontend/src/components/viewers/__snapshots__/Tensorboard.test.tsx.snap

frontend/server/k8s-helper.ts

frontend/src/components/viewers/Tensorboard.tsx

Bobgy

Thanks for the contribution!
/lgtm
/approve

jingzhang36 · 2019-12-24T09:51:05Z

/lgtm

dldaisy · 2019-12-24T10:01:18Z

/retest

dldaisy · 2019-12-24T10:26:07Z

/test kubeflow-pipeline-sample-test

jingzhang36 · 2019-12-24T11:38:15Z

/lgtm

jingzhang36 · 2019-12-24T11:38:47Z

/approve

k8s-ci-robot · 2019-12-24T11:38:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, jingzhang36

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:

~~backend/OWNERS~~ [jingzhang36]
~~frontend/OWNERS~~ [Bobgy,jingzhang36]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nielsmeima · 2020-07-01T12:46:18Z

Currently, the versions to pick from are hard-coded, however newer tags are available (which might solves issues like #2509). Could these be loaded dynamically?

EDIT: can confirm that using the tag 2.1.1 issue #2509 gets resolved.

* Support select tensorflow image for tensorboard * modify test for tensorflow version select * delete not available image entry * Support tensorflow image selection to run tensorboard * format code with prettier * use HasPrefix instead of regexp * delete * modified tensorboard test * delete tensorboard * modify typo * test tensorboard * tensorboard test * fuck * fuck2 * modify test * modify typo in tensorboard hint * npm run format * modify tensorboard snapshot * compatible with previous kfp version. Allow vacant tensorflowImage field. * add 2 tests for dialog * modify default tensorflow image to 1.13.2 * merge get version and get tensorboard; let --bind_all support tensorboard3.x * modify reconciler.go * reconciler rollback * modify corresponding test for --bind_all * modify requested chances 12/23 * formControl sorted alphabetically * select sorted alphabetically * modify details from PR request 12/24 * moidfy format * modify details 12/23 * modify snapshot * retest * retest

dldaisy added 4 commits November 29, 2019 11:02

Support select tensorflow image for tensorboard

556ace1

modify test for tensorflow version select

f9259bf

delete not available image entry

8d541fb

Support tensorflow image selection to run tensorboard

e0d2f4b

k8s-ci-robot added the size/XXL label Dec 3, 2019

k8s-ci-robot added the needs-ok-to-test label Dec 3, 2019

k8s-ci-robot requested review from Bobgy and jingzhang36 December 3, 2019 08:25

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Dec 3, 2019

Bobgy reviewed Dec 3, 2019

View reviewed changes

dldaisy added 2 commits December 4, 2019 15:02

format code with prettier

c89a22b

use HasPrefix instead of regexp

1bf624a

k8s-ci-robot added size/XL and removed size/XXL labels Dec 4, 2019

Bobgy reviewed Dec 4, 2019

View reviewed changes

dldaisy added 11 commits December 6, 2019 10:46

delete

301ec41

modified tensorboard test

07ba4fc

delete tensorboard

68ae3bf

modify typo

45af753

test tensorboard

324bc83

Merge remote-tracking branch 'upstream/master'

81dab3b

Merge remote-tracking branch 'upstream/master'

9578720

tensorboard test

48bffbc

fuck

65ff6bb

fuck2

df9c9fa

modify test

9a22416

Bobgy reviewed Dec 24, 2019

View reviewed changes

dldaisy added 2 commits December 24, 2019 13:54

modify details from PR request 12/24

8801bb2

moidfy format

4bf953a

Bobgy reviewed Dec 24, 2019

View reviewed changes

modify details 12/23

34e26ed

Bobgy approved these changes Dec 24, 2019

View reviewed changes

k8s-ci-robot assigned Bobgy Dec 24, 2019

k8s-ci-robot added the lgtm label Dec 24, 2019

Bobgy added the area/frontend label Dec 24, 2019

modify snapshot

fec3711

k8s-ci-robot removed the lgtm label Dec 24, 2019

k8s-ci-robot assigned jingzhang36 Dec 24, 2019

k8s-ci-robot added the lgtm label Dec 24, 2019

retest

04048ac

k8s-ci-robot removed the lgtm label Dec 24, 2019

retest

f0e1790

k8s-ci-robot added the lgtm label Dec 24, 2019

k8s-ci-robot added the approved label Dec 24, 2019

k8s-ci-robot merged commit 5c27fa8 into kubeflow:master Dec 24, 2019

This was referenced Jan 21, 2020

Allow choosing tensorboard version through UI #2514

Closed

Let user choose the version of Tensorboard deployed by UI #2185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support choosing tensorboard version from UI #2690

Support choosing tensorboard version from UI #2690

dldaisy commented Dec 3, 2019 •

edited by jlewi

Loading

googlebot commented Dec 3, 2019

k8s-ci-robot commented Dec 3, 2019

Bobgy commented Dec 3, 2019

Bobgy left a comment

Bobgy Dec 3, 2019

dldaisy Dec 4, 2019

Bobgy Dec 4, 2019

dldaisy Dec 5, 2019

Bobgy Dec 4, 2019

dldaisy Dec 5, 2019

Bobgy Dec 4, 2019

dldaisy Dec 5, 2019

Bobgy Dec 4, 2019

Bobgy Dec 4, 2019

Bobgy Dec 4, 2019

dldaisy Dec 5, 2019

Bobgy Dec 5, 2019

dldaisy commented Dec 24, 2019

Bobgy Dec 24, 2019

Bobgy left a comment

jingzhang36 commented Dec 24, 2019

dldaisy commented Dec 24, 2019

dldaisy commented Dec 24, 2019

jingzhang36 commented Dec 24, 2019

jingzhang36 commented Dec 24, 2019

k8s-ci-robot commented Dec 24, 2019

nielsmeima commented Jul 1, 2020 •

edited

Loading

Support choosing tensorboard version from UI #2690

Support choosing tensorboard version from UI #2690

Conversation

dldaisy commented Dec 3, 2019 • edited by jlewi Loading

googlebot commented Dec 3, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

k8s-ci-robot commented Dec 3, 2019

Bobgy commented Dec 3, 2019

Bobgy 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

dldaisy commented Dec 24, 2019

Choose a reason for hiding this comment

Bobgy left a comment

Choose a reason for hiding this comment

jingzhang36 commented Dec 24, 2019

dldaisy commented Dec 24, 2019

dldaisy commented Dec 24, 2019

jingzhang36 commented Dec 24, 2019

jingzhang36 commented Dec 24, 2019

k8s-ci-robot commented Dec 24, 2019

nielsmeima commented Jul 1, 2020 • edited Loading

dldaisy commented Dec 3, 2019 •

edited by jlewi

Loading

nielsmeima commented Jul 1, 2020 •

edited

Loading