KEMBAR78
timeseries: preserve select all on page when new runs arrive by psybuzz · Pull Request #4888 · tensorflow/tensorboard · GitHub
Skip to content

Conversation

@psybuzz
Copy link
Contributor

@psybuzz psybuzz commented Apr 20, 2021

Before, the auto-selection logic was:

  • For the 1st load, auto-select only if runCount <= N
  • On subsequent loads, new runs are always unselected

For users who frequently want new runs to be automatically
selected, we

  • Increase N to 500
  • Show a toast when the run set exceeds N, informing the user
    that new runs might not be selected.

image

Manually tested by running a script, pressing the in-app reload.

import tensorflow as tf

for i in range(50):
  writer = tf.compat.v2.summary.create_file_writer('./logs' + str(i))
  writer.set_as_default()
  tf.summary.scalar("cool tag", 1, step=1)

Googlers, see b/184284696.

@google-cla google-cla bot added the cla: yes label Apr 20, 2021
@psybuzz psybuzz changed the title bug(timeseries): preserve select all on page when new runs arrive timeseries: preserve select all on page when new runs arrive Apr 20, 2021
@psybuzz psybuzz marked this pull request as ready for review April 20, 2021 22:09
@psybuzz psybuzz requested a review from stephanwlee April 20, 2021 22:09
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

Overall approach SGTM. Thanks for addressing this!

Comment on lines 109 to 113
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we undo this and use the alerts module? This is taking the imperative approach of a view instead of more declarative version: e.g., you are using business logic to determine whether to show a snackbar or not then are making an imperative API call to the snackbar. Instead, the view should react to the data change which we already have in the alerts component. Just add an alert in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@psybuzz psybuzz requested a review from stephanwlee April 26, 2021 17:25
Comment on lines 101 to 107
Copy link
Contributor

Choose a reason for hiding this comment

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

this is effectively creating a composite action; please do this in the alerts feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per offline discussion, moved the composite action to RunsTable, and added a comment to indicate that this should be considered an exception, which goes against recommended patterns.

Comment on lines +410 to +412
this.store.dispatch(
alertActions.alertReported({localizedMessage: text})
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, does any asynchronusity disrupt the UX? Adding what is effectively setTimeout(0) would make me feel tiny bit better about the synchronous composite action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually added it to try it out, and the UX seems fine, e.g. Alert still appears and the rest of the UI is functional. Not sure how valuable it is to go add setTimeouts to all cases like these, but perhaps worth considering in a separate PR.

Comment on lines +388 to +389
* `alertFromAction`. Unfortunately, those currently have no way of knowing
* whether a run table is actually shown (with checkboxes), so we make a
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so we are 100% aligned; since the view is a representation of the state in the store, you can reason with the state in the store to know. However, it is quite cumbersome and, now, you have to ingrain presentation logic into the store which is very unpleasant.

});
}

this.store.dispatch(runTableShown({experimentIds: this.experimentIds}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, when did we do this :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Googlers, see original cl/330969935. Back then, it was also noted that this is kind of an anti-pattern, but a tradeoff was made for convenience. For the same reason as you mentioned, ingraining presentation logic into the store would be unpleasant, so this tradeoff still probably makes sense.

@psybuzz psybuzz merged commit 304b1d3 into tensorflow:master Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants