Changes reorder column events to use source and destination #6733

hoonji · 2024-01-25T09:45:12Z

Motivation for features / changes

To prepare for showing shared hparam columns across runs and scalar tables, we need to unify the column re-ordering logic to use the same source/destination parameters (introduced in #6727).

Technical description of changes

Changes the DataTable component (shared by runs and scalar tables) to use the new ReorderColumnEvent type when column order is changed. The changes propagate to scalar card scalar column editor containers. Most of the changes are getting the tests to pass with the new signature.
Changes the metrics reducers to correctly handle these new events. Logic is mostly unchanged, except that sort order will now be preserved even after column toggle (previously, toggling would group all disabled columns together, which destroys relative column order

misc:

creates moveColumn utility to abstract shared logic between the hparams and metrics reducers. Removes redundant tests in hparams reducer

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

Unit tests pass
Manually tested sorting behavior

hoonji · 2024-01-25T09:46:45Z

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

@@ -18,10 +18,8 @@
 <tb-data-table
  [headers]="columnHeaders"
  [sortingInfo]="sortingInfo"
-  [columnCustomizationEnabled]="columnCustomizationEnabled"


hoonji · 2024-01-25T09:47:41Z

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

  (sortDataBy)="sortDataBy.emit($event)"
-  (orderColumns)="orderColumns($event)"
-  (removeColumn)="removeColumn.emit($event)"


scalar tables shouldn't be able to remove columns, just hide them. But there will be no functional change atm because this is currently disabled.

Once the scalar card show hparam columns I would expect to be able to remove them from there.

I agree that showing both "hide" and "remove" buttons is also reasonable, but showing both in the same context menu might make a lot of users wondering what the difference is. Meanwhile, allowing only one of these functionalities per table might make the distinction a bit clearer.

In any case, I think it's important to gather more feedback from the team and from users on what the ideal context menu setup should be. It should be trivial to add/remove items later as necessary.

We definitely shouldn't show both in the same context menu. I'm not sure splitting the functionality by table will be clearer for users.

hoonji · 2024-01-25T09:48:46Z

.../webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ng.html

@@ -21,16 +21,16 @@
    (selectedTabChange)="tabChange($event)"
  >
    <mat-tab [label]="'Single'">
-      <ngContext


ngContext doesn't seem to be a valid directive? replaced with ng-container, which is.

hoonji · 2024-01-25T09:51:33Z

...board/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts

@@ -48,16 +56,27 @@ import {DataTableMode} from '../../../../widgets/data_table/types';
 export class ScalarColumnEditorContainer {
  constructor(private readonly store: Store<State>) {}

-  readonly singleHeaders$ = this.store.select(getSingleSelectionHeaders);
-  readonly rangeHeaders$ = this.store.select(getRangeSelectionHeaders);
+  readonly singleHeaders$ = this.store


runs are now hidden in the edit menu so they will no longer be sortable or hidable

hoonji · 2024-01-25T09:52:27Z

tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts

@@ -235,7 +235,8 @@ export class OSSSettingsConverter extends SettingsConverter<
    if (
      Array.isArray(backendSettings.singleSelectionHeaders) &&
      // If the settings stored in the backend are invalid, reset back to default.
-      backendSettings.singleSelectionHeaders[0].name !== undefined
+      backendSettings.singleSelectionHeaders[0].name !== undefined &&
+      backendSettings.singleSelectionHeaders[0].type === 'RUN'


adds check when loading from existing localstorage as downstream sorting logic depends on RUN being first

hoonji · 2024-01-25T09:52:58Z

tensorboard/webapp/widgets/data_table/utils.ts

+  return newColumns;
+}
+
+export const DataTableUtils = {


this export style is ugly, but necessary due to #5991

tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts

tensorboard/webapp/metrics/store/metrics_reducers.ts

rileyajones · 2024-01-25T16:47:18Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

+    (state, {source, destination, side, dataTableMode}) => {
+      let headers =
+        dataTableMode === DataTableMode.RANGE
+          ? [...state.rangeSelectionHeaders]


Previously the headers were retrieved from the action, now they are being retrieved from the current state. How can the headers ever change?

To clarify:

dataTableColumnEdited is currently only used for reordering

runs and hparams features already define separate actions for reordering and adding (e.g. runsTableHeaderAdded, runsTableHeaderOrderChanged)

To prevent confusion (and for consistency) I'll change dataTableColumnEdited -> dataTableColumnOrderChanged

(a separate dataTableColumnAdded action will be added in an upcoming PR)

In that case can you headers from the props?

Wait sorry - I need to correct myself. I did add a dataTableColumnAdded in #6737, but it's unused because we don't actually have a feature for adding standard columns, only hparam columns. Thus, only the hparams action dashboardHparamColumnAdded will be used to add new columns.

Regarding passing headers to the props, I propose that an action signature like dashboardHparamColumnAdded, i.e. only specifying {column, nextTo, side} and letting the reducer figure out how put it in the right place, is more in line with redux best practices, which advocate putting logic in reducers, not components; passing in headers via the action implies that the component must do some data manipulation first.

(Note that the previously implemented runs table add action already uses the "put logic in reducers" pattern, albeit with a slightly different signature.)

I fully agree with everything in this statement.
The thing that I find confusing is that the action dataTableColumnEdited takes HeaderEditInfo as props which has an unused attribute headers

tensorboard/webapp/metrics/store/metrics_reducers.ts

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

...board/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts

...board/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts

tensorboard/webapp/widgets/data_table/data_table_component.ts

rileyajones

Thanks for sticking with this. I would really like to see the props changed on dataTableColumnEdited but otherwise things look pretty good.
go/small-cls

hoonji · 2024-01-31T06:56:17Z

I would really like to see the props changed on dataTableColumnEdited

Ack - headers should be gone in the latest version

go/small-cls

Yeah my bad, I didn't realize the code would get so bloated after tests 😭 I'll take care to make them smaller next time. Thanks btw for the comprehensive reviews on these huge batches of code!

## Motivation for features / changes Recently merged hparam column PRs (#6732, #6733, #6736) will cause build and lint errors during 1p sync. ## Technical description of changes - import map from rxjs/operators instead of rxjs - `DataTableUtils` -> `dataTableUtils` - use proper string signature on CardStateMap in tests

hoonji added 4 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

hoonji commented Jan 25, 2024

View reviewed changes

hoonji requested a review from rileyajones January 25, 2024 09:56

hoonji added 4 commits January 25, 2024 19:30

Fixes lint

f3130d0

Runs buildifier

a9c53c3

Turns groupColumns into a projector function helper

f17fe22

Merge branch 'hparam_common_selectors' into hparam_update_reorder

555a84e

rileyajones reviewed Jan 26, 2024

View reviewed changes

hoonji added 4 commits January 27, 2024 13:28

Changes ColumnGroup to enum

ec93a93

Merge branch 'hparam_common_selectors' into hparam_update_reorder

cff6ce3

Applies fixes

62f4099

Fixes lint error

e96b32a

hoonji requested a review from rileyajones January 29, 2024 11:08

rileyajones approved these changes Jan 30, 2024

View reviewed changes

Base automatically changed from hparam_common_selectors to master January 31, 2024 06:33

Rename dataTableColumnEdited to dataTableColumnOrderChanged in tests

8f4a38b

Merge branch 'master' into hparam_update_reorder

4bd7dcd

hoonji merged commit 0d76a12 into master Jan 31, 2024
16 checks passed

hoonji deleted the hparam_update_reorder branch January 31, 2024 12:58

hoonji mentioned this pull request Jan 31, 2024

Fixes sync errors #6739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes reorder column events to use source and destination #6733

Changes reorder column events to use source and destination #6733

hoonji commented Jan 25, 2024 •

edited

Loading

hoonji Jan 25, 2024

rileyajones Jan 25, 2024

hoonji Jan 25, 2024

rileyajones Jan 26, 2024

hoonji Jan 29, 2024

rileyajones Jan 29, 2024

hoonji Jan 25, 2024 •

edited

Loading

hoonji Jan 25, 2024

hoonji Jan 25, 2024

hoonji Jan 25, 2024

rileyajones Jan 25, 2024

hoonji Jan 29, 2024

hoonji Jan 29, 2024

rileyajones Jan 29, 2024

hoonji Jan 30, 2024 •

edited

Loading

rileyajones Jan 30, 2024

rileyajones left a comment

hoonji commented Jan 31, 2024

Changes reorder column events to use source and destination #6733

Changes reorder column events to use source and destination #6733

Conversation

hoonji commented Jan 25, 2024 • edited Loading

Motivation for features / changes

Technical description of changes

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

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 25, 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

hoonji Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileyajones left a comment

Choose a reason for hiding this comment

hoonji commented Jan 31, 2024

hoonji commented Jan 25, 2024 •

edited

Loading

hoonji Jan 25, 2024 •

edited

Loading

hoonji Jan 30, 2024 •

edited

Loading