Displays hparams in scalar card tables #6737

hoonji · 2024-01-29T13:05:52Z

Motivation for features / changes

To complete hparams in time series phase 2, namely to show selected hparams in the scalar card tables similarly to how they're currently shown in runs tables.

Technical description of changes

Enables hparam column adding and filtering in scalar card tables. Triggers the dashboardHparamColumnAdded action on table add button click, or on "insert left/right" context menu option click
Shows appropriate hparam data in scalar card table rows
Enables removing columns using scalar card table context menus

Screenshots of UI changes (or N/A)

Synced hparams:

Insert, remove, filter context menus in scalar tables:

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

Manually tested scalar table hparam add, sort, filter, remove

hoonji · 2024-01-29T13:06:56Z

tensorboard/webapp/widgets/data_table/data_table_component.ts

@@ -303,7 +303,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit {
    return (
      this.selectableColumns &&
      this.selectableColumns.length &&
-      this.contextMenuHeader?.movable
+      this.contextMenuHeader?.type === 'HPARAM'


Note: as of now, it's only possible for hparam columns to be added

hoonji · 2024-01-29T13:24:47Z

tensorboard/webapp/metrics/views/main_view/card_grid_component.scss

@@ -48,7 +48,6 @@ card-view {
  @include tb-theme-foreground-prop(border, border, 1px solid);
  border-radius: 4px;
  box-sizing: border-box;
-  contain: layout paint;


Because modal components (like the column header context menus) are defined as descendants of card-view, it's impossible to make them show on top of the next card-view, which has its own overriding stacking context:

Removing the stacking context seems the only way to solve the modal z-index issue without re-implementing the modal to render in the overlay container, but I'm a bit concerned about the performance implications of removing contain: paint - IIUC, pages with many open panels with many line charts may experience some additional render lag?

@bmd3k I see that you last worked on this part of the code - do you happen to have any insight into the safety of removing contain: paint? FWIW there seems to be no noticeable difference when testing with my limited data, but just want to double check.

Ok after additional experimentation, I'm nearly certain that removing contain: paint is a very bad idea - it makes interactions like opening/closing the settings menu much more clunky even with my small dataset. I'll remove the change from this PR.

This likely means that I'll have to reimplement the context menu to prevent it being covered in the minimized table views. I hope this is minor enough to not be considered a blocker :'( (users can simply maximize the table first as a workaround)

rileyajones · 2024-01-29T22:45:47Z

tensorboard/webapp/metrics/store/metrics_selectors.ts

+          newColumn.removable = false;
+          newColumn.hidable = true;


I don't understand why we don't want hparam columns in the scalar card data table to be removed, can you please explain that a bit?

It's

vs

I thought maybe limiting hide to only scalar tables and remove to only runs tables might be conceptually easier to understand, but in hindsight the second option actually doesn't look so confusing after all. I'll go ahead and enable both buttons, and see what users think.

As discussed offline, the final version will only use the "remove" action; no more "hide"

tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts

rileyajones · 2024-01-29T23:01:37Z

tensorboard/webapp/widgets/data_table/data_table_component.ts

@@ -316,6 +316,15 @@ export class DataTableComponent implements OnDestroy, AfterContentInit {
    );
  }

+  contextMenuHideColumn() {
+    if (this.contextMenuHeader === undefined) {


Suggested change

if (this.contextMenuHeader === undefined) {

if (!this.contextMenuHeader) {

tensorboard/webapp/widgets/data_table/data_table_test.ts

rileyajones · 2024-01-29T23:05:12Z

tensorboard/webapp/widgets/data_table/data_table_component.ts

@@ -303,7 +303,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit {
    return (
      this.selectableColumns &&
      this.selectableColumns.length &&
-      this.contextMenuHeader?.movable


This line is important. We have pinned columns (e.g. the runs column) which we do not want to move. If we allow columns to be inserted to the left of these columns they will functionally have moved.

Good point. Re-added this and updated the corresponding test.

rileyajones · 2024-01-29T23:06:42Z

tensorboard/webapp/widgets/data_table/data_table_component.ng.html

@@ -17,6 +17,14 @@
    <div *ngIf="isContextMenuEmpty()" class="no-actions-message">
      No Actions Available
    </div>
+    <button
+      *ngIf="contextMenuHeader?.hidable"


I don't think we should ever show both "Hide" and "Remove"

Suggested change

*ngIf="contextMenuHeader?.hidable"

*ngIf="!contextMenuHeader?.removable && contextMenuHeader?.hidable"

tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts

tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts

rileyajones · 2024-01-29T23:27:52Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts

@@ -674,11 +716,55 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
    );
  }

-  editColumnHeaders(headerEditInfo: HeaderEditInfo) {
-    this.store.dispatch(dataTableColumnOrderChanged(headerEditInfo));
+  editColumnHeaders({


The UI will not stop my from switching the position of an hparam and non hparam column but I believe this is not supported. I think we either need to stop the user from dragging columns beyond groups or else support mixing groups.

Imagine the following initial column order

[Run Name, Experiment Name, HP1, HP2]

Now the user drags the Run Name Column to the end

[Run Name, Experiment Name, HP1, HP2, Run Name] ^-----------------------------------^

The resulting state will be quite different from what they expect

[Experiment Name, Run Name, HP1, HP2]

This isn't really the best example since Run isn't movable, but it should highlight the issue.

I agree this is ideal. Turns out to be pretty simple to implement - just prevent dragEnter if headers are in different groups! We conveniently already have a function for checking groups from #6732

Implemented this behavior and added tests.

Result (it's hard to see the mouse, but it's hovering over the columns on the right):

Awesome, I'm glad that turned out to be so easy to solve

rileyajones · 2024-01-29T23:28:10Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html

@@ -28,7 +33,6 @@
        [header]="header"
        [sortingInfo]="sortingInfo"
        [hparamsEnabled]="hparamsEnabled"
-        disableContextMenu="true"


I think this change should be behind a feature flag.

SG, I'll put it behind the existing enableScalarColumnCustomization rather than creating a separate flag just for disabling context menus (context menus will be enabled when enableScalarColumnCustomization is enabled)

This PR has gotten really big (see go/small-cls) so I'd rather you not continue adding too much to it but this isn't really a good choice of feature flags because it is enabled by default internally.
cs/learning/brain/tensorboard/service/tbcorp/ui/app/feature_flag/feature_flag_metadata.ts

Created another PR for the feature flag: #6742

rileyajones · 2024-01-30T21:22:45Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html

@@ -28,7 +33,6 @@
        [header]="header"
        [sortingInfo]="sortingInfo"
        [hparamsEnabled]="hparamsEnabled"
-        disableContextMenu="true"


This PR has gotten really big (see go/small-cls) so I'd rather you not continue adding too much to it but this isn't really a good choice of feature flags because it is enabled by default internally.
cs/learning/brain/tensorboard/service/tbcorp/ui/app/feature_flag/feature_flag_metadata.ts

rileyajones · 2024-01-30T21:23:54Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts

@@ -674,11 +716,55 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
    );
  }

-  editColumnHeaders(headerEditInfo: HeaderEditInfo) {
-    this.store.dispatch(dataTableColumnOrderChanged(headerEditInfo));
+  editColumnHeaders({


Awesome, I'm glad that turned out to be so easy to solve

rileyajones · 2024-01-30T21:26:22Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts

@@ -251,6 +264,13 @@ export class ScalarCardDataTable {
              selectedStepData[header.name] =
                closestEndPoint.value - closestStartPoint.value;
              continue;
+            case ColumnHeaderType.HPARAM:
+              let runId =


Suggested change

let runId =

const runId =

whoops, done!

tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts

hoonji · 2024-02-01T09:33:09Z

tensorboard/webapp/metrics/store/metrics_selectors_test.ts

@@ -1819,7 +1819,7 @@ describe('metrics selectors', () => {
            singleSelectionHeaders,
            rangeSelectionHeaders,
            cardStateMap: {
-              card1: {
+              'card1': {


suppresses 1p tslint

hoonji · 2024-02-01T13:25:22Z

@rileyajones as discussed in the meeting, I did away with the "hide" concept and merged it with "remove" (which I see now is much cleaner both conceptually and technically). PTAL! Some of the comments point to old code - please refer to the latest version (it's the same code, just without the hide)

…ext menus (#6742) ## Motivation for features / changes As per discussion in #6737, adding a feature flag in front of scalar table context menus before we implement them.

## Motivation for features / changes Splits DataTableComponent's context menu functionality into its own component to be compatible with future updates that will enable dynamic custom modal creation (for alleviating [stacking context issues](#6737 (comment))) ## Technical description of changes - No new functionality - simply moves the context menu related template and logic to a new ContextMenuComponent - Parameterizes some context menu tests for legibility. ## Detailed steps to verify changes work correctly (as executed by you) - Tested manually that all context menu features work as before

## Motivation for features / changes The current Custom Modal can't display modals outside of their ancestors' stacking context (e.g. in scalar card tables: #6737 (comment) ), which significantly hinders scalar table context menu functionality. It also has some minor tricky issues that are difficult to fix like some modals lingering when another one is opened: ![image](https://github.com/tensorflow/tensorboard/assets/736199/934b1d0a-5650-47e2-94f4-e8836c1a6ab4) ## Technical description of changes - Before: Custom modals were defined in a consumer component's template. The modals were embedded views of the consumer component (e.g. DataTableComponent), which prevented them from being displayed outside the table's stacking context, e.g. outside of a scalar card table - After: Custom modals are still defined in a consumer component's template, but wrapped in ng-templates. They are dynamically created using a newly defined CustomModal service. When the appropriate CustomModal service method is a called, the modal template is attached as an embedded view using CDK Overlay, which places it in a top-level overlay container, freeing it from all stacking context issues. CustomModalComponent is completely subsumed by Overlay and is thus deleted. Only the CustomModal service will be required going forward. ## Detailed steps to verify changes work correctly (as executed by you) Manually tested all custom modal features in runs table, filter bar, scalar card table (Need to set query param `enableScalarColumnContextMenus=true` to test scalar table context menu behavior)

## Motivation for features / changes Splits DataTableComponent's context menu functionality into its own component to be compatible with future updates that will enable dynamic custom modal creation (for alleviating [stacking context issues](tensorflow#6737 (comment))) ## Technical description of changes - No new functionality - simply moves the context menu related template and logic to a new ContextMenuComponent - Parameterizes some context menu tests for legibility. ## Detailed steps to verify changes work correctly (as executed by you) - Tested manually that all context menu features work as before

## Motivation for features / changes The current Custom Modal can't display modals outside of their ancestors' stacking context (e.g. in scalar card tables: tensorflow#6737 (comment) ), which significantly hinders scalar table context menu functionality. It also has some minor tricky issues that are difficult to fix like some modals lingering when another one is opened: ![image](https://github.com/tensorflow/tensorboard/assets/736199/934b1d0a-5650-47e2-94f4-e8836c1a6ab4) ## Technical description of changes - Before: Custom modals were defined in a consumer component's template. The modals were embedded views of the consumer component (e.g. DataTableComponent), which prevented them from being displayed outside the table's stacking context, e.g. outside of a scalar card table - After: Custom modals are still defined in a consumer component's template, but wrapped in ng-templates. They are dynamically created using a newly defined CustomModal service. When the appropriate CustomModal service method is a called, the modal template is attached as an embedded view using CDK Overlay, which places it in a top-level overlay container, freeing it from all stacking context issues. CustomModalComponent is completely subsumed by Overlay and is thus deleted. Only the CustomModal service will be required going forward. ## Detailed steps to verify changes work correctly (as executed by you) Manually tested all custom modal features in runs table, filter bar, scalar card table (Need to set query param `enableScalarColumnContextMenus=true` to test scalar table context menu behavior)

hoonji added 21 commits January 24, 2024 20:59

Adds shared hparam related common selectors

89fc04a

Fixes test description

bed9604

Fixes formatting

9eef734

Changes reorder column events to use source and destination

f72b2a2

Fixes lint

f3130d0

Runs buildifier

a9c53c3

Turns groupColumns into a projector function helper

f17fe22

Merge branch 'hparam_common_selectors' into hparam_update_reorder

555a84e

Makes runs table use hparam store for hparam columns

6e45e6e

Changes ColumnGroup to enum

ec93a93

Merge branch 'hparam_common_selectors' into hparam_update_reorder

cff6ce3

Applies comments

c17894d

Merge branch 'hparam_update_reorder' into hparam_runs_hparams

694251c

Applies comments

ff8c096

Adds hparams to scalar tables

999bffc

Fixes card view stacking context

d3f2cb9

Applies fixes

62f4099

Fixes lint error

e96b32a

Fixes lint error

7f39b54

Merge branch 'hparam_update_reorder' into hparam_runs_hparams

ef301e5

Merge branch 'hparam_runs_hparams' into hparam_scalar_table_hparams

06b550e

hoonji commented Jan 29, 2024

View reviewed changes

hoonji requested a review from rileyajones January 29, 2024 13:07

hoonji commented Jan 29, 2024

View reviewed changes

Revert card view layout paint

dd390f0

hoonji mentioned this pull request Jan 29, 2024

Updates edit column menu to support hparam columns #6738

Open

Move contain layout paint to original position

8f95404

rileyajones reviewed Jan 29, 2024

View reviewed changes

hoonji mentioned this pull request Jan 30, 2024

Changes reorder column events to use source and destination #6733

Merged

Applies fixes

c58c14d

rileyajones reviewed Jan 30, 2024

View reviewed changes

Base automatically changed from hparam_runs_hparams to master January 31, 2024 14:09

hoonji added 6 commits February 1, 2024 01:04

Adds remove column functionality to scalar tables

4d7c07a

Merge branch 'master' into hparam_scalar_table_hparams

2a7bb5c

Misc fixes

2e07ef8

Removes hidable contxt menu option and combines it with removable

e29efc3

Merge branch 'hparam_remove_hidable' into hparam_scalar_table_hparams

129b298

Removes hide column functionality from data tables

0428494

hoonji changed the base branch from master to hparam_remove_hidable February 1, 2024 09:32

hoonji commented Feb 1, 2024

View reviewed changes

hoonji added 2 commits February 1, 2024 21:56

Adds enableScalarColumnContextMenus feature flag

1a1f08b

Reverts merge error

85fa49a

hoonji mentioned this pull request Feb 1, 2024

Adds feature flag to control whether or not to show scalar table context menus #6742

Merged

hoonji changed the base branch from hparam_remove_hidable to hparam_add_feature_Flag February 1, 2024 13:05

hoonji added 2 commits February 1, 2024 22:07

Merge branch 'hparam_add_feature_Flag' into hparam_scalar_table_hparams

3dfae7c

Fixes lint

a8a1b5a

hoonji requested a review from rileyajones February 1, 2024 13:25

rileyajones approved these changes Feb 5, 2024

View reviewed changes

Base automatically changed from hparam_add_feature_Flag to master February 6, 2024 01:46

Merge branch 'master' into hparam_scalar_table_hparams

9585fb2

hoonji merged commit a69cf8b into master Feb 6, 2024
16 checks passed

hoonji deleted the hparam_scalar_table_hparams branch February 6, 2024 12:42

hoonji mentioned this pull request Mar 15, 2024

Splits context menu into separate component #6788

Merged

hoonji mentioned this pull request Mar 19, 2024

Changes custom modal to be created dynamically #6799

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Displays hparams in scalar card tables #6737

Displays hparams in scalar card tables #6737

hoonji commented Jan 29, 2024 •

edited

Loading

hoonji Jan 29, 2024 •

edited

Loading

hoonji Jan 29, 2024

hoonji Jan 29, 2024 •

edited

Loading

rileyajones Jan 29, 2024

hoonji Jan 30, 2024

hoonji Feb 1, 2024

rileyajones Jan 29, 2024

hoonji Jan 30, 2024

rileyajones Jan 29, 2024

hoonji Jan 30, 2024

rileyajones Jan 29, 2024

rileyajones Jan 29, 2024

hoonji Jan 30, 2024 •

edited

Loading

rileyajones Jan 30, 2024

rileyajones Jan 29, 2024

hoonji Jan 30, 2024

rileyajones Jan 30, 2024

hoonji Feb 1, 2024

rileyajones Jan 30, 2024

rileyajones Jan 30, 2024

rileyajones Jan 30, 2024

hoonji Feb 1, 2024

hoonji Feb 1, 2024

hoonji commented Feb 1, 2024

	if (this.contextMenuHeader === undefined) {
	if (!this.contextMenuHeader) {

	*ngIf="contextMenuHeader?.hidable"
	*ngIf="!contextMenuHeader?.removable && contextMenuHeader?.hidable"

Displays hparams in scalar card tables #6737

Displays hparams in scalar card tables #6737

Conversation

hoonji commented Jan 29, 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)

hoonji Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoonji Jan 29, 2024 • edited Loading

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

hoonji Jan 30, 2024 • edited Loading

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

hoonji commented Feb 1, 2024

hoonji commented Jan 29, 2024 •

edited

Loading

hoonji Jan 29, 2024 •

edited

Loading

hoonji Jan 29, 2024 •

edited

Loading

hoonji Jan 30, 2024 •

edited

Loading