Introduce the run GET parameter #505

chihuahua · 2017-09-08T04:33:10Z

Introduced a run GET parameter for the graph dashboard. The
value of this parameter is the name of the dataset (run) to load. This
parameter lets users send links to other users of the graph dashboard
that load specific datasets. Fixes #306.

When the user changes the dataset in the graph dashboard UI, the URL
parameter correspondingly changes. I tested this behavior out using the
various runs of the scalars demo. The correct dataset loads given the
selected_dataset parameter, and the URL changes when the dataset is
changed in the UI.

For instance, http://localhost:6006/#graphs&selected_dataset=temperature%3At0%3D270%2CtA%3D270%2CkH%3D0.001 makes the dashboard load the correct dataset.

If no dataset with the specified name can be found, we show a dialog
that notifies the user. This avoids misleading the user.

Set the type of the graph dashboard's _datasets property to Array. It had
previously been Object (incorrect).

wchargin · 2017-09-08T12:48:51Z

tensorboard/components/tf_storage/storage.ts

@@ -54,6 +54,11 @@ export let TAB = '__tab__';
 export let DISAMBIGUATOR = 'disambiguator';

 /**
+ * The (string) name for the run of the selected dataset in the graph dashboard.
+ */
+export const SELECTED_DATASET = 'selected_dataset';


Why isn't this just called 'run'? We call it a run everywhere else in the UI (within this plugin and across other plugins); I don't see the need for new terminology.

A little conflict I'm grappling with is that the graph can load many "datasets" (as used in the standalone explorer). Within the graph dashboard, those datasets happen to correspond to runs.

The selected_dataset (or run) param is only used by the graph dashboard. If we rename to run, that piece of the URL would appear when the user navigates to other dashboards as well (but is not actually used and is semantically awkward given how the user can select many runs at once). What do you think is the best way to go there?

wchargin · 2017-09-08T12:55:14Z

tensorboard/plugins/graph/tf_graph_dashboard/tf-graph-dashboard.html

@@ -193,6 +199,7 @@
      value: true,  // TODO(@chihuahua): Reconcile this setting with Health Pills.
      readOnly: true,
    },
+    _selectedDataset: Number,


This doesn't look like it responds to changes in the hash. Other plugins have bidirectional linkage.

Is there a reason that you don't want to use

properties: { _selectedDatasetName: { type: String, notify: true, value: storage.getStringInitializer('run', /*default=*/'', /*useLocalStorage=*/false), observer: '_selectedDatasetNameObserver', }, }, _selectedDatasetNameObserver: storage.getStringObserver( 'run', /*default=*/'', /*useLocalStorage=*/false),

that then has an additional observer on _selectedDatasetName that looks up the index and sets _selectedDataset accordingly?

Done. This took some debugging to figure out. The 1st argument to getStringObserver must also be the name of the polymer module property to sync with:

tensorboard/tensorboard/components/tf_storage/storage.ts

Line 392 in 41aa124

const newVal = this[propertyName];

I might change that in a PR because it caught me a little by surprise.

Specifically, I think we can change getStringObserver's signature to be

getStringObserver( propertyName: string, defaultVal: string, useLocalStorage = false, polymerModuleProperty?: string): Function

and polymerModuleProperty would only apply if useLocalStorage were false.

wchargin · 2017-09-08T12:57:22Z

tensorboard/plugins/graph/tf_graph_dashboard/tf-graph-dashboard.html

+            SELECTED_DATASET, /*useLocalStorage=*/ false);
+        if (selectedDatasetRun) {
+          let datasetFound = false;
+          for (let i = 0; i < datasets.length; i++) {


It looks like this is all just

const dataset = datasets.findIndex(x => x.name === selectedDatasetRun); if (dataset === -1) { // error code } else { this.set('_selectedDataset', dataset); }

?

wchargin

I agree that the requirement that the store key and the property name be equal is surprising and seriously limiting, and would support a change.

Some API comments for that change:

Why should polymerModuleProperty only apply when useLocalStorage is false? If we’re writing to local storage, we might just as well want the local storage key to differ from the key on the Polymer object.
Can we rename polymerModuleProperty to polymerProperty?
Can we rename propertyName to key, just to avoid confusion? (I know it’s existing.)
Note that you will also have to change getStringInitializer.
I presume that you will also change the utility functions for other types.

This API is bloating, and positional arguments with default values are not usually the best solution. You might consider changing the function’s signature to the following:

interface Options<T> = {
  defaultValue?: T;
  polymerProperty?: string;
  useLocalStorage?: boolean;
}

function getStringObserver(key: string, options?: Options<T>) {
  return getObserver(key, {
    defaultValue: '',
    polymerProperty: key,
    useLocalStorage: false,
    ...options,
  });
}

This can be factored into

export const [getStringObserver, getStringInitializer] =
    makeBindings(getString, setString, '');
export const [getNumberObserver, getNumberInitializer] =
    makeBindings(getNumber, setNumber, 0);
// etc.

function makeBindings<T>(get: ..., set: ..., defaultDefaultValue: T) {
    return [..., ...];
}

Perhaps I can throw up a proof-of-concept and you can see what you think?

wchargin · 2017-09-10T17:26:52Z

tensorboard/components/tf_storage/storage.ts

@@ -54,6 +54,11 @@ export let TAB = '__tab__';
 export let DISAMBIGUATOR = 'disambiguator';

 /**
+ * The (string) name for the run of the selected dataset in the graph dashboard.
+ */
+export const RUN = 'run';


This constant shouldn't be in storage.ts; there's no reason that the core storage module should need to know about the graph dashboard. Instead, how about const RUN_STORAGE_KEY = 'RUN'; in tf-graph-dashboard.html? (No need to export anything; it can be local within the <script>.)

wchargin · 2017-09-10T17:29:12Z

tensorboard/plugins/graph/tf_graph_dashboard/tf-graph-dashboard.html

@@ -193,6 +200,17 @@
      value: true,  // TODO(@chihuahua): Reconcile this setting with Health Pills.
      readOnly: true,
    },
+    run: {


Can you add a comment here noting the name of this property must coincide with the value of storage.RUN?

Can rebase onto #509 (which has been merged) to avoid this complexity.

Done. Also checked that correct behavior was preserved (The URL changes when the run changes. The correct run is loaded when run= is set. A non-existent run triggers the dialog.).

wchargin · 2017-09-10T19:17:15Z

See #509 per discussion above.

googlebot · 2017-09-11T02:34:15Z

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 by someone other than the pull request submitter. We need to confirm that they're okay 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 the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-09-11T02:48:09Z

CLAs look good, thanks!

chihuahua

The API comments you noted above SG! I really like how we can now clearly set the polymer property and the string key to use. And passing those options in an object indeed seems better than adding optional function parameters.

chihuahua · 2017-09-11T02:07:11Z

tensorboard/components/tf_storage/storage.ts

@@ -54,6 +54,11 @@ export let TAB = '__tab__';
 export let DISAMBIGUATOR = 'disambiguator';

 /**
+ * The (string) name for the run of the selected dataset in the graph dashboard.
+ */
+export const RUN = 'run';


chihuahua · 2017-09-11T02:51:31Z

tensorboard/plugins/graph/tf_graph_dashboard/tf-graph-dashboard.html

@@ -193,6 +200,17 @@
      value: true,  // TODO(@chihuahua): Reconcile this setting with Health Pills.
      readOnly: true,
    },
+    run: {


Done. Also checked that correct behavior was preserved (The URL changes when the run changes. The correct run is loaded when run= is set. A non-existent run triggers the dialog.).

wchargin · 2017-09-11T03:24:09Z

Done. Also checked that correct behavior was preserved (The URL changes when the run changes. The correct run is loaded when run= is set. A non-existent run triggers the dialog.).

Fantastic. Thanks.

chihuahua added the plugin:graph label Sep 8, 2017

chihuahua requested review from jart, wchargin and dsmilkov September 8, 2017 04:33

chihuahua changed the title ~~Let the user chose a run (graph dataset) to load~~ Introduce the selected_dataset GET parameter Sep 8, 2017

wchargin reviewed Sep 8, 2017

View reviewed changes

chihuahua force-pushed the chihuahua-bind-graph-run branch from 5fb4c97 to 30c4eec Compare September 9, 2017 03:48

wchargin approved these changes Sep 10, 2017

View reviewed changes

chihuahua force-pushed the chihuahua-bind-graph-run branch 2 times, most recently from 7212a51 to 0a41ec1 Compare September 11, 2017 02:43

Use storage object

9a9aeb7

chihuahua force-pushed the chihuahua-bind-graph-run branch from 0a41ec1 to 9a9aeb7 Compare September 11, 2017 02:48

chihuahua commented Sep 11, 2017

View reviewed changes

chihuahua merged commit 91811c7 into master Sep 11, 2017

chihuahua deleted the chihuahua-bind-graph-run branch September 11, 2017 02:56

chihuahua changed the title ~~Introduce the selected_dataset GET parameter~~ Introduce the run GET parameter Sep 11, 2017

chihuahua mentioned this pull request Sep 13, 2017

Can I specify a "Run" by url? #518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the run GET parameter #505

Introduce the run GET parameter #505

chihuahua commented Sep 8, 2017 •

edited

Loading

wchargin Sep 8, 2017

chihuahua Sep 8, 2017

wchargin Sep 8, 2017

chihuahua Sep 9, 2017 •

edited

Loading

chihuahua Sep 9, 2017 •

edited

Loading

wchargin Sep 8, 2017

chihuahua Sep 8, 2017

wchargin left a comment

wchargin Sep 10, 2017

chihuahua Sep 11, 2017

wchargin Sep 10, 2017

wchargin Sep 10, 2017 •

edited

Loading

chihuahua Sep 11, 2017

wchargin commented Sep 10, 2017

googlebot commented Sep 11, 2017

googlebot commented Sep 11, 2017

chihuahua left a comment

chihuahua Sep 11, 2017

chihuahua Sep 11, 2017

wchargin commented Sep 11, 2017

Introduce the run GET parameter #505

Introduce the run GET parameter #505

Conversation

chihuahua commented Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chihuahua Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

chihuahua Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin 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

wchargin Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin commented Sep 10, 2017

googlebot commented Sep 11, 2017

googlebot commented Sep 11, 2017

chihuahua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin commented Sep 11, 2017

chihuahua commented Sep 8, 2017 •

edited

Loading

chihuahua Sep 9, 2017 •

edited

Loading

chihuahua Sep 9, 2017 •

edited

Loading

wchargin Sep 10, 2017 •

edited

Loading