[Global pins] Saving pins to localStorage #6819

roseayeon · 2024-04-01T07:20:43Z

Motivation for features / changes

[Global pins] To extend pinned card info available globally, this PR saves pinned info to localStorage

Technical description of changes

Introduced savedPinsDataSource to store pinned card tag information in local storage.
- Since the focus in on saving scalar cards, only the tag name is persisted.
- Implemented methods saveScalarPin, removeScalarPin, and getSavedScalarPins.
Added addOrRemovePin$ effect in the metrics/effects/index.ts
- When the actions. cardPinStateToggled is triggered, find the card info in the visibeCardFetchInfos , and the scalar pin info is saved or removed accordingly.

Screenshots of UI changes (or N/A)

N/A

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

Unit tests pass

Alternate designs / implementations considered (or N/A)

N/A

tensorboard/webapp/metrics/data_source/saved_pins_data_source_test.ts

hoonji

Looks great! Note that @rileyajones is OOO atm.

tensorboard/webapp/metrics/effects/index.ts

tensorboard/webapp/metrics/effects/metrics_effects_test.ts

bmd3k

Approved! But please consider protecting with a feature flag before merging.

bmd3k · 2024-04-02T11:27:29Z

tensorboard/webapp/metrics/data_source/BUILD

+    ],
+    deps = [
+        ":saved_pins_data_source",
+        "//tensorboard/webapp/angular:expect_angular_core_testing",


If you haven't already, please don't forget to use copybara_presubmit to import the changes into a local google3 repo and run TAP on it.

(I have no reason to believe that the import will fail but I just want to make sure you've done that step).

Thank you! I almost forgot to use copybara_presubmit. I checked that TAP passed.

bmd3k · 2024-04-02T11:30:56Z

tensorboard/webapp/metrics/effects/index.ts

+  private readonly addOrRemovePin$ = this.actions$.pipe(
+    ofType(actions.cardPinStateToggled),
+    withLatestFrom(this.getVisibleCardFetchInfos()),
+    map(([{cardId, canCreateNewPins, wasPinned}, fetchInfos]) => {


Can we protect this with a feature flag until you have the entire feature built and tested end-to-end?

I'm worried you will build out the rest of the feature and test it and find some reason to make changes to the storage layer. However, people will already have started writing their pins to local storage (even if they don't see the rest of this feature) and perhaps what they store will be incompatible with the final version.

SGTM! Added a new feature flag

bmd3k · 2024-04-02T11:37:24Z

tensorboard/webapp/metrics/effects/index.ts

@@ -261,6 +263,23 @@ export class MetricsEffects implements OnInitEffects {
    })
  );

+  private readonly addOrRemovePin$ = this.actions$.pipe(
+    ofType(actions.cardPinStateToggled),
+    withLatestFrom(this.getVisibleCardFetchInfos()),


(For a followup PR):

Can we check getShouldPersistSettings() before deciding whether to write state?

tensorboard/tensorboard/webapp/persistent_settings/_redux/persistent_settings_selectors.ts

Line 26 in 04a064f

export const getShouldPersistSettings = createSelector(

The idea is:

Internally we have a share feature (in tbcorp).

If you load a shared tensorboard, we don't believe any decisions you make should be persisted in local storage and affect your regular usage.

getShouldPersistSettings captures this (it returns true by default but returns false if you load a shared dashboard)

IIUC, if a shared TensorBoard is used, we should not store user activities in local storage. To determine this, we need to check if getShouldPersistSettings is true. If so, we can store in the local storage; otherwise, we should not. Is this correct?

Yes, that is exactly what I would recommend.

roseayeon

Thanks for the detailed comments! It was so helpful

roseayeon · 2024-04-02T16:47:19Z

tensorboard/webapp/metrics/effects/index.ts

@@ -261,6 +263,23 @@ export class MetricsEffects implements OnInitEffects {
    })
  );

+  private readonly addOrRemovePin$ = this.actions$.pipe(
+    ofType(actions.cardPinStateToggled),
+    withLatestFrom(this.getVisibleCardFetchInfos()),


IIUC, if a shared TensorBoard is used, we should not store user activities in local storage. To determine this, we need to check if getShouldPersistSettings is true. If so, we can store in the local storage; otherwise, we should not. Is this correct?

roseayeon · 2024-04-02T16:48:43Z

tensorboard/webapp/metrics/data_source/BUILD

+    ],
+    deps = [
+        ":saved_pins_data_source",
+        "//tensorboard/webapp/angular:expect_angular_core_testing",


Thank you! I almost forgot to use copybara_presubmit. I checked that TAP passed.

roseayeon · 2024-04-02T16:54:04Z

tensorboard/webapp/metrics/effects/index.ts

+  private readonly addOrRemovePin$ = this.actions$.pipe(
+    ofType(actions.cardPinStateToggled),
+    withLatestFrom(this.getVisibleCardFetchInfos()),
+    map(([{cardId, canCreateNewPins, wasPinned}, fetchInfos]) => {


SGTM! Added a new feature flag

## Motivation for features / changes Following #6819 [Global pins] To extend pinned card info available globally , this PR load saved pins in the localStorage. ## Technical description of changes * 3e5b052 Introduced new action called`metricsUnresolvedPinnedCardsAdded`. * When `metricsUnresolvedPinnedCardsAdded` is dispatched, the reducer adds the provided card info to the `state. unresolvedImportedPinnedCards`. * aae9be4 Added `loadSavedPins$` effect in the `metrics/effects/index.ts` * When `coreActions.pluginsListingLoaded` is triggered, it loads saved scalar pins in the localStorage and dispatch `metricsUnresolvedPinnedCardsAdded` with saved pinned cards. * Added `getShouldPersistSettings()` checking logic (metioned in #6819 (comment)) * 2dc49aa To use selector globally, added `persistent_settings_selectors` to the `webapp/selectors`. * Add checking `getShouldPersistSettings` in the `loadSavedPins$` and `addOrRemovePin$`. ## Screenshots of UI changes (or N/A) N/A ## Detailed steps to verify changes work correctly (as executed by you) * Unit test pass & TAP presubmit pass ## Alternate designs / implementations considered (or N/A) N.A

## Motivation for features / changes [Global pins] To extend pinned card info available globally, this PR saves pinned info to localStorage ## Technical description of changes * Introduced `savedPinsDataSource` to store pinned card tag information in local storage. * Since the focus in on saving scalar cards, only the tag name is persisted. * Implemented methods `saveScalarPin`, `removeScalarPin`, and `getSavedScalarPins`. * Added `addOrRemovePin$` effect in the `metrics/effects/index.ts` * When the `actions. cardPinStateToggled` is triggered, find the card info in the `visibeCardFetchInfos` , and the scalar pin info is saved or removed accordingly. ## Screenshots of UI changes (or N/A) N/A ## Detailed steps to verify changes work correctly (as executed by you) * Unit tests pass ## Alternate designs / implementations considered (or N/A) N/A

## Motivation for features / changes Following tensorflow#6819 [Global pins] To extend pinned card info available globally , this PR load saved pins in the localStorage. ## Technical description of changes * 3e5b052 Introduced new action called`metricsUnresolvedPinnedCardsAdded`. * When `metricsUnresolvedPinnedCardsAdded` is dispatched, the reducer adds the provided card info to the `state. unresolvedImportedPinnedCards`. * aae9be4 Added `loadSavedPins$` effect in the `metrics/effects/index.ts` * When `coreActions.pluginsListingLoaded` is triggered, it loads saved scalar pins in the localStorage and dispatch `metricsUnresolvedPinnedCardsAdded` with saved pinned cards. * Added `getShouldPersistSettings()` checking logic (metioned in tensorflow#6819 (comment)) * 2dc49aa To use selector globally, added `persistent_settings_selectors` to the `webapp/selectors`. * Add checking `getShouldPersistSettings` in the `loadSavedPins$` and `addOrRemovePin$`. ## Screenshots of UI changes (or N/A) N/A ## Detailed steps to verify changes work correctly (as executed by you) * Unit test pass & TAP presubmit pass ## Alternate designs / implementations considered (or N/A) N.A

roseayeon added 2 commits April 1, 2024 06:48

defined savedPinsDataSource and added related tests

4ed2c0b

add saving pin logic in the metrics_effect

586e503

roseayeon requested review from hoonji and rileyajones April 1, 2024 07:20

fix BUILD lint error

c29f40d

hoonji reviewed Apr 2, 2024

View reviewed changes

tensorboard/webapp/metrics/data_source/saved_pins_data_source_test.ts Outdated Show resolved Hide resolved

hoonji requested changes Apr 2, 2024

View reviewed changes

tensorboard/webapp/metrics/effects/index.ts Outdated Show resolved Hide resolved

tensorboard/webapp/metrics/effects/metrics_effects_test.ts Outdated Show resolved Hide resolved

tensorboard/webapp/metrics/effects/metrics_effects_test.ts Outdated Show resolved Hide resolved

rename effect to addOrRemovePin$ and remove some redundant test codes

5602eb9

roseayeon requested review from bmd3k and hoonji and removed request for rileyajones April 2, 2024 06:06

hoonji approved these changes Apr 2, 2024

View reviewed changes

bmd3k approved these changes Apr 2, 2024

View reviewed changes

roseayeon added 3 commits April 2, 2024 15:37

change localStorage to window.localStorage

6fdd93e

define enableGlobalPins feature flag

4806818

protect writing localstorage with feature flag

0bc3a45

roseayeon force-pushed the global-pin branch from 4a93c3d to 0bc3a45 Compare April 2, 2024 16:52

roseayeon commented Apr 2, 2024

View reviewed changes

roseayeon merged commit 4f4801c into tensorflow:master Apr 3, 2024
13 checks passed

roseayeon mentioned this pull request Apr 3, 2024

[Global pins] Load saved pins in the localStorage #6821

Merged

roseayeon deleted the global-pin branch April 5, 2024 04:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Global pins] Saving pins to localStorage #6819

[Global pins] Saving pins to localStorage #6819

roseayeon commented Apr 1, 2024 •

edited

Loading

hoonji left a comment

bmd3k left a comment

bmd3k Apr 2, 2024

roseayeon Apr 2, 2024

bmd3k Apr 2, 2024

roseayeon Apr 2, 2024

bmd3k Apr 2, 2024

roseayeon Apr 2, 2024

bmd3k Apr 2, 2024

roseayeon left a comment

roseayeon Apr 2, 2024

roseayeon Apr 2, 2024

roseayeon Apr 2, 2024

[Global pins] Saving pins to localStorage #6819

[Global pins] Saving pins to localStorage #6819

Conversation

roseayeon commented Apr 1, 2024 • edited Loading

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)

hoonji left a comment

Choose a reason for hiding this comment

bmd3k 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

roseayeon 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

roseayeon commented Apr 1, 2024 •

edited

Loading