-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Displays hparams in scalar card tables #6737
Conversation
@@ -303,7 +303,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { | |||
return ( | |||
this.selectableColumns && | |||
this.selectableColumns.length && | |||
this.contextMenuHeader?.movable | |||
this.contextMenuHeader?.type === 'HPARAM' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: as of now, it's only possible for hparam columns to be added
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
newColumn.removable = false; | ||
newColumn.hidable = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
@@ -316,6 +316,15 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { | |||
); | |||
} | |||
|
|||
contextMenuHideColumn() { | |||
if (this.contextMenuHeader === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (this.contextMenuHeader === undefined) { | |
if (!this.contextMenuHeader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -303,7 +303,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { | |||
return ( | |||
this.selectableColumns && | |||
this.selectableColumns.length && | |||
this.contextMenuHeader?.movable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Re-added this and updated the corresponding test.
@@ -17,6 +17,14 @@ | |||
<div *ngIf="isContextMenuEmpty()" class="no-actions-message"> | |||
No Actions Available | |||
</div> | |||
<button | |||
*ngIf="contextMenuHeader?.hidable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should ever show both "Hide" and "Remove"
*ngIf="contextMenuHeader?.hidable" | |
*ngIf="!contextMenuHeader?.removable && contextMenuHeader?.hidable" |
tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts
Outdated
Show resolved
Hide resolved
@@ -674,11 +716,55 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { | |||
); | |||
} | |||
|
|||
editColumnHeaders(headerEditInfo: HeaderEditInfo) { | |||
this.store.dispatch(dataTableColumnOrderChanged(headerEditInfo)); | |||
editColumnHeaders({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I'm glad that turned out to be so easy to solve
@@ -28,7 +33,6 @@ | |||
[header]="header" | |||
[sortingInfo]="sortingInfo" | |||
[hparamsEnabled]="hparamsEnabled" | |||
disableContextMenu="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change should be behind a feature flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created another PR for the feature flag: #6742
@@ -28,7 +33,6 @@ | |||
[header]="header" | |||
[sortingInfo]="sortingInfo" | |||
[hparamsEnabled]="hparamsEnabled" | |||
disableContextMenu="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -674,11 +716,55 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { | |||
); | |||
} | |||
|
|||
editColumnHeaders(headerEditInfo: HeaderEditInfo) { | |||
this.store.dispatch(dataTableColumnOrderChanged(headerEditInfo)); | |||
editColumnHeaders({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I'm glad that turned out to be so easy to solve
@@ -251,6 +264,13 @@ export class ScalarCardDataTable { | |||
selectedStepData[header.name] = | |||
closestEndPoint.value - closestStartPoint.value; | |||
continue; | |||
case ColumnHeaderType.HPARAM: | |||
let runId = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let runId = | |
const runId = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, done!
@@ -1819,7 +1819,7 @@ describe('metrics selectors', () => { | |||
singleSelectionHeaders, | |||
rangeSelectionHeaders, | |||
cardStateMap: { | |||
card1: { | |||
'card1': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suppresses 1p tslint
@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) |
## 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)
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
dashboardHparamColumnAdded
action on table add button click, or on "insert left/right" context menu option clickScreenshots 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)