[Global Pins] Added a disabling global pins feature #6831

roseayeon · 2024-04-16T05:26:14Z

Motivation for features / changes

Following #6828,

Since global pins are applied automatically, this PR allows users disable the global pin feature.

Technical description of changes

2cf2b11 Added savingPinsEnabled in the MetricsSettings

Also added a selector getting settings.savingPinsEnabled value from the store.

dec6a63 Introduced new action metricsEnableSavingPinsToggled

If metricsEnableSavingPinsToggled is dispatched, it toggles the savingPinsEnabled store value.

6ea22fd Added a checkbox UI to the settings view component.

Also added a feature flag guard to the component. If check box is clicked, it dispatched metricsEnableSavingPinsToggled action.

568febf Added a new effect disableSavingPins

When metricsEnableSavingPinsToggled action is called, this effect will call removeAllScalarPins method if savingPinsEnabled value is false.

bbf9050 Added saving pins feature in persistent settings to preserve user preferences.

3ff7cd2 Add buildMetricsSettingsOverrides util in the testing to use in tbcorp

Screenshots of UI changes (or N/A)

Detailed steps to verify changes work correctly (as executed by you)

Unit test passes + created a cl/625208577 to pass TAP Presubmit tests

Alternate designs / implementations considered (or N/A)

N/A

Action Items in the next PR

In the next PR, I'll add a "confirmation dialog" that appears when the user unchecks the checkbox to make the user aware that local storage is disappearing.

Add the confirmation dialog

roseayeon · 2024-04-16T09:01:26Z

tensorboard/webapp/metrics/testing.ts

@@ -59,6 +59,7 @@ export function buildMetricsSettingsState(
    imageContrastInMilli: 123,
    imageShowActualSize: true,
    histogramMode: HistogramMode.OFFSET,
+    savingPinsEnabled: true,


Change in MetricsSettings required tbcorp change so I created cl/625208577

bmd3k · 2024-04-16T15:13:34Z

tensorboard/webapp/metrics/views/right_pane/settings_view_component.ng.html

+    <mat-icon
+      class="info"
+      svgIcon="help_outline_24px"
+      title="When saving pins are enabled, pinned cards will be shared across multiple experiments."


Nit: Instead of "shared across" perhaps "visible across"?

bmd3k · 2024-04-16T15:14:48Z

tensorboard/webapp/metrics/views/right_pane/settings_view_component.scss

+  align-items: center;
+  display: flex;
+
+  .info {


This seems to be the same as L86-92. Can we just move the rule to a place where it only needs to be defined once? (make it a sibling to .saving-pins and .scalars-partition-x rules perhaps?)

Applied! Changed it to be defined with .scalars-partition-x using a comma.

roseayeon

Thanks for the quick review!

roseayeon · 2024-04-17T05:23:00Z

tensorboard/webapp/metrics/views/right_pane/settings_view_component.ng.html

+    <mat-icon
+      class="info"
+      svgIcon="help_outline_24px"
+      title="When saving pins are enabled, pinned cards will be shared across multiple experiments."


roseayeon · 2024-04-17T05:27:57Z

tensorboard/webapp/metrics/views/right_pane/settings_view_component.scss

+  align-items: center;
+  display: flex;
+
+  .info {


Applied! Changed it to be defined with .scalars-partition-x using a comma.

hoonji · 2024-04-17T09:19:12Z

Sorry for the delay! I should be able to do a full review later tonight.

hoonji · 2024-04-17T09:50:14Z

tensorboard/webapp/metrics/store/metrics_selectors_test.ts

@@ -1298,6 +1298,18 @@ describe('metrics selectors', () => {
      );
      expect(selectors.getMetricsCardMinWidth(state)).toBe(400);
    });
+
+    it('returns savingPinsEnabled when called getMetricsSavingPinsEnabled', () => {


nit: the test isn't very robust, as many different expressions could evaluate to false. A easy, succinct way to test both true/false can be parameterized testing

hoonji · 2024-04-17T13:27:13Z

tensorboard/webapp/metrics/effects/index.ts

+    })
+  );
+
+  private readonly disableSavingPins$ = this.actions$.pipe(


I was a bit confused by the naming at first because the effect doesn't actually disable (disabling is done elsewhere), this just removes pins.

WDYT of a naming that describes the actual action, like removeOnDisable?

I changed to removeSavedPinsOnDisable 👍

hoonji · 2024-04-17T13:33:56Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

+  on(actions.metricsEnableSavingPinsToggled, (state) => {
+    const nextSavingPinsEnabled = !(
+      state.settingOverrides.savingPinsEnabled ??
+      state.settings.savingPinsEnabled


[optional] I'm curious, did you find out anything about the "legacy reasons" why we need settings as well as settingsOverrides?

I may be mistaken, but based on my understanding, the settings property is used for setting default values and loading persistent settings, while the property settingsOverrides is used when a user makes changes, such as toggling a checkbox.

In the code, the settingsOverrides is used to override the original setting store value. I'm not sure why these properties are separated, but that's what I discovered while examining the code.

Thanks, that makes it clearer!

hoonji · 2024-04-17T13:38:39Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -1232,6 +1232,21 @@ describe('metrics reducers', () => {
      expect(thirdState.settings.hideEmptyCards).toBe(false);
      expect(thirdState.settingOverrides.hideEmptyCards).toBe(false);
    });
+
+    it('changes savingPinsEnabled on metricsEnableSavingPinsToggled', () => {


Since this tests a "toggle", could we have at least two test cases minimum here? One changing from true -> false, another from false -> true (parameterized tests may help - please see other comment)

I added some states to make it true -> false -> true.

nit: interleaving act and assert steps is explicitly forbidden by our style guide and I think we should follow it unless there's a very strong reason not to. Can we just create separate tests for true->false and false->true?

Applied. Thanks for reminding me! I just followed this interleaving act and assert steps because it was commonly used in this test codebase

Thanks for the cleanup! My readability meta-mentor linked me to this when I asked if consistency is important: https://peps.python.org/pep-0008/#a-foolish-consistency-is-the-hobgoblin-of-little-minds

My stance now is the right thing > consistency, unless inconsistency causes big problems

roseayeon

Thanks for the review!

hoonji · 2024-04-19T01:40:08Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -1232,6 +1232,21 @@ describe('metrics reducers', () => {
      expect(thirdState.settings.hideEmptyCards).toBe(false);
      expect(thirdState.settingOverrides.hideEmptyCards).toBe(false);
    });
+
+    it('changes savingPinsEnabled on metricsEnableSavingPinsToggled', () => {


nit: interleaving act and assert steps is explicitly forbidden by our style guide and I think we should follow it unless there's a very strong reason not to. Can we just create separate tests for true->false and false->true?

hoonji · 2024-04-19T02:57:13Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -1232,6 +1232,21 @@ describe('metrics reducers', () => {
      expect(thirdState.settings.hideEmptyCards).toBe(false);
      expect(thirdState.settingOverrides.hideEmptyCards).toBe(false);
    });
+
+    it('changes savingPinsEnabled on metricsEnableSavingPinsToggled', () => {


Thanks for the cleanup! My readability meta-mentor linked me to this when I asked if consistency is important: https://peps.python.org/pep-0008/#a-foolish-consistency-is-the-hobgoblin-of-little-minds

My stance now is the right thing > consistency, unless inconsistency causes big problems

…6840) ## Motivation for features / changes This PR fixes the renaming in #6831 and adds test cases to make the test robust. ## Technical description of changes * Tried to rename `disableSavingPins` to `removeSavedPinsOnDisable`, but #6831 accidentally changed `removeAllPins` to `removeSavedPinsOnDisable`. ## Screenshots of UI changes (or N/A) ## Detailed steps to verify changes work correctly (as executed by you) * Added test cases + cl TAP presubmit passes ## Alternate designs / implementations considered (or N/A)

…in feature (#6843) ## Motivation for features / changes Regarding #6831 , this PR adds confirmation dialog" that appears when the user clears the checkbox to make the user aware that local storage is disappearing. ## Technical description of changes 0e63b07 created a `saving pins dialog` component 05e06f5 Created a `saving pins checkbox` component * The Save Pin checkbox prevents the default click and must be done after confirmation in the modal. So I added `MAT_CHECKBOX_DEFAULT_OPTIONS` (https://v15.material.angular.io/comComponents/checkbox/overview#click-action-config) to prevent the default click event. However, if I add this directly to the `settings view component`, all checkboxes will be disabled, including other unwanted ones. So, I created a separate checkbox for the Save Pin checkbox. Please let me know is there any other good way to do this. 6ba778e Make the dialog shows up when user clears the checkbox * Passes to `onEnableSavingPinsToggled` whether the checkbox is selected. * If the checkbox is selected and user clicks it, it means that the user disables the global pin, so a dialog box is displayed. * If the checkbox is not checked but is clicked, it means the user is trying to enable a global pin and no dialog box will be displayed. * `onEnableSavingPinsToggled` and `isSavingPinsEnabled` is passed to `saving-pins-checkbox` component. ## Screenshots of UI changes (or N/A) ![image](https://github.com/tensorflow/tensorboard/assets/24772412/eb442565-4f39-45e7-b893-4866ee3fc1e4) ## Detailed steps to verify changes work correctly (as executed by you) unit test passes + TAP presubmit passes ## Alternate designs / implementations considered (or N/A)

## Motivation for features / changes Following tensorflow#6828, Since global pins are applied automatically, this PR allows users disable the global pin feature. ## Technical description of changes 2cf2b11 Added `savingPinsEnabled` in the `MetricsSettings` * Also added a selector getting `settings.savingPinsEnabled` value from the store. dec6a63 Introduced new action `metricsEnableSavingPinsToggled` * If `metricsEnableSavingPinsToggled` is dispatched, it toggles the `savingPinsEnabled` store value. 6ea22fd Added a checkbox UI to the settings view component. * Also added a feature flag guard to the component. If check box is clicked, it dispatched `metricsEnableSavingPinsToggled` action. 568febf Added a new effect `disableSavingPins` * When `metricsEnableSavingPinsToggled` action is called, this effect will call `removeAllScalarPins` method if `savingPinsEnabled` value is false. bbf9050 Added saving pins feature in persistent settings to preserve user preferences. 3ff7cd2 Add buildMetricsSettingsOverrides util in the testing to use in tbcorp ## Screenshots of UI changes (or N/A) ![image](https://github.com/tensorflow/tensorboard/assets/24772412/874dbc2b-f01f-4112-b34b-6b07330ea31f) ## Detailed steps to verify changes work correctly (as executed by you) Unit test passes + created a cl/625208577 to pass TAP Presubmit tests ## Alternate designs / implementations considered (or N/A) N/A ## Action Items in the next PR In the next PR, I'll add a "confirmation dialog" that appears when the user unchecks the checkbox to make the user aware that local storage is disappearing. - [ ] Add the confirmation dialog

…ensorflow#6840) ## Motivation for features / changes This PR fixes the renaming in tensorflow#6831 and adds test cases to make the test robust. ## Technical description of changes * Tried to rename `disableSavingPins` to `removeSavedPinsOnDisable`, but tensorflow#6831 accidentally changed `removeAllPins` to `removeSavedPinsOnDisable`. ## Screenshots of UI changes (or N/A) ## Detailed steps to verify changes work correctly (as executed by you) * Added test cases + cl TAP presubmit passes ## Alternate designs / implementations considered (or N/A)

…in feature (tensorflow#6843) ## Motivation for features / changes Regarding tensorflow#6831 , this PR adds confirmation dialog" that appears when the user clears the checkbox to make the user aware that local storage is disappearing. ## Technical description of changes 0e63b07 created a `saving pins dialog` component 05e06f5 Created a `saving pins checkbox` component * The Save Pin checkbox prevents the default click and must be done after confirmation in the modal. So I added `MAT_CHECKBOX_DEFAULT_OPTIONS` (https://v15.material.angular.io/comComponents/checkbox/overview#click-action-config) to prevent the default click event. However, if I add this directly to the `settings view component`, all checkboxes will be disabled, including other unwanted ones. So, I created a separate checkbox for the Save Pin checkbox. Please let me know is there any other good way to do this. 6ba778e Make the dialog shows up when user clears the checkbox * Passes to `onEnableSavingPinsToggled` whether the checkbox is selected. * If the checkbox is selected and user clicks it, it means that the user disables the global pin, so a dialog box is displayed. * If the checkbox is not checked but is clicked, it means the user is trying to enable a global pin and no dialog box will be displayed. * `onEnableSavingPinsToggled` and `isSavingPinsEnabled` is passed to `saving-pins-checkbox` component. ## Screenshots of UI changes (or N/A) ![image](https://github.com/tensorflow/tensorboard/assets/24772412/eb442565-4f39-45e7-b893-4866ee3fc1e4) ## Detailed steps to verify changes work correctly (as executed by you) unit test passes + TAP presubmit passes ## Alternate designs / implementations considered (or N/A)

roseayeon marked this pull request as draft April 16, 2024 07:41

roseayeon marked this pull request as ready for review April 16, 2024 08:50

roseayeon commented Apr 16, 2024

View reviewed changes

roseayeon requested review from hoonji and bmd3k April 16, 2024 09:28

bmd3k approved these changes Apr 16, 2024

View reviewed changes

roseayeon force-pushed the disable-global-pins branch from f23dbee to 74cc62d Compare April 17, 2024 05:42

roseayeon commented Apr 17, 2024

View reviewed changes

hoonji requested changes Apr 17, 2024

View reviewed changes

roseayeon added 7 commits April 18, 2024 14:18

Add savingPinsEnabled to MetricSettings and add a related selector

2cf2b11

Add metricsEnableSavingPinsToggled action and related reducer

dec6a63

Add enable saving pins checkbox in the settings view component

6ea22fd

Add disableSavingPins effect

568febf

Add savingPinsEnabled to PersistableSettings

bbf9050

Change info message and move scss rule to be defined once

abb477b

Add buildMetricsSettingsOverrides for testing

3ff7cd2

roseayeon force-pushed the disable-global-pins branch from 370132c to 38d9b67 Compare April 18, 2024 14:19

roseayeon commented Apr 18, 2024

View reviewed changes

roseayeon requested a review from hoonji April 18, 2024 14:32

hoonji requested changes Apr 19, 2024

View reviewed changes

add some test cases to make test robust

bb40a23

roseayeon force-pushed the disable-global-pins branch from 38d9b67 to bb40a23 Compare April 19, 2024 02:51

hoonji approved these changes Apr 19, 2024

View reviewed changes

roseayeon merged commit 9ff1585 into tensorflow:master Apr 19, 2024

roseayeon mentioned this pull request Apr 23, 2024

[Global pins] Correct the incorrect name change and add test cases #6840

Merged

roseayeon mentioned this pull request Apr 26, 2024

[Global Pins] Shows a confirmation dialog when disabling the global pin feature #6843

Merged

[Global Pins] Added a disabling global pins feature #6831

[Global Pins] Added a disabling global pins feature #6831

Uh oh!

Conversation

roseayeon commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for features / changes

Technical description of changes

Screenshots of UI changes (or N/A)

Detailed steps to verify changes work correctly (as executed by you)

Alternate designs / implementations considered (or N/A)

Action Items in the next PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roseayeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoonji commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoonji Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roseayeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roseayeon commented Apr 16, 2024 •

edited

Loading

hoonji Apr 17, 2024 •

edited

Loading