HParams: Create filter dialog component #6493

rileyajones · 2023-07-13T21:17:29Z

Motivation for features / changes

We want to be able to filter the runs table by hparam. The existing logic for this is handled by a bunch of inner components found in the runs_table_component which makes it hard to share. In this PR I am replicating that UI with some enhancements in a standalone component which will later be used by the data_table_component.

See #6488 for screenshots and a more complete description of what is being added

Alternate designs / implementations considered (or N/A)

I'm not sure that this component should be part of the data table but that is simplest place to put it for now.

tensorboard/webapp/hparams/_types.ts

tensorboard/webapp/runs/views/runs_table/runs_table_container.ts

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

tensorboard/webapp/widgets/data_table/BUILD

tensorboard/webapp/widgets/data_table/filter_dialogue.ts

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

tensorboard/webapp/widgets/data_table/BUILD

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

tensorboard/webapp/widgets/data_table/types.ts

tensorboard/webapp/hparams/BUILD

tensorboard/webapp/runs/data_source/runs_data_source_types.ts

tensorboard/webapp/widgets/data_table/types.ts

bmd3k · 2023-07-14T17:27:14Z

tensorboard/webapp/widgets/data_table/filter_dialogue_test.ts

+    ).find((elem) =>
+      elem.nativeElement.innerHTML.includes('Include Undefined')
+    )!;
+    includeUndefinedCheckbox.query(By.css('label')).nativeElement.click();


Is there a way to use Component Harnesses in the tests? Its current best practice but will also possibly make it easier to switch to the new components introduced in Angular 15.

Did you give Component Harnesses a try?

I did manage to get this working with harnesses

tensorboard/webapp/hparams/BUILD

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

tensorboard/webapp/hparams/_types.ts

tensorboard/webapp/runs/data_source/runs_data_source_types.ts

tensorboard/webapp/widgets/data_table/BUILD

tensorboard/webapp/widgets/data_table/types.ts

bmd3k · 2023-07-21T16:28:34Z

tensorboard/webapp/widgets/data_table/filter_dialogue_test.ts

+    ).find((elem) =>
+      elem.nativeElement.innerHTML.includes('Include Undefined')
+    )!;
+    includeUndefinedCheckbox.query(By.css('label')).nativeElement.click();


Did you give Component Harnesses a try?

tensorboard/webapp/runs/views/runs_table/runs_table_container.ts

bmd3k

Thanks its looking pretty good. All that remains are some requests for increased test coverage and clarity.

tensorboard/webapp/widgets/data_table/filter_dialog_test.ts

bmd3k · 2023-08-14T21:05:05Z

tensorboard/webapp/widgets/data_table/filter_dialog_test.ts

+    fixture.componentInstance.discreteValueFilter = 'ba';
+    fixture.detectChanges();
+    expect(
+      (await rootLoader.getAllHarnesses(MatCheckboxHarness)).length


I would be more confident in the test if you actually checked that it's the 'bar' and 'baz' boxes shown.

This would still make me more confident. (You resolved this but I didn't see corresponding changes).

I'm not sure how this has happened but this comment is on an outdated version of the file.

Clicking your comment allows me to see the code you are commenting on:

However, clicking the "Files Changed" tab I see a much different file (where I did address your comment).

tensorboard/webapp/widgets/data_table/filter_dialog_test.ts

bmd3k

A few more comments to be addressed, please.

bmd3k · 2023-08-15T14:23:28Z

tensorboard/webapp/widgets/data_table/filter_dialog_test.ts

+    );
+    await checkbox.uncheck();
+
+    expect(filterValues).toEqual([2, 2, 4]);


I would be more confident in the test if you confirmed the output of the EventEmitter after every click. You say "Unchecking an unchecked box should not trigger an event" but the test does not prove this - it actually allows for the possibility of "Checking an unchecked box does not trigger an event" (which is a bug).

Alternatively, another way to make me more confident in the test is to just get rid of L166,167 and then we have only three uncheck/check calls and its a bit easier to conclude that the filterValues on L175 correspond to those three clicks. In which case, if you think the test on L167 is important to test, consider moving it to its own test case.

This is my second draft of this. In the original I did check after each event but I thought the amount of setup required for each action made it hard to read. I'm going to put this in a loop instead but I plan to maintain the practice of checking the values in an array as I do find it more readable.

bmd3k · 2023-08-15T17:07:56Z

tensorboard/defs/defs.bzl

@@ -72,8 +71,8 @@ def tf_js_binary(
        # the global level and tends to overwrite `window` functions. "iife" is
        # just a thin wrapper around "esm" (it adds 11 bytes) and doesn't
        # suffer from the same overwriting problem.
-        format="iife",
-        minify= False if dev_mode_only else True,
+        format = "iife",


You have unrelated changes here.

The observer effect (aka ide autoformat) is alive and strong. Sorry I missed this.

bmd3k · 2023-08-15T17:10:02Z

tensorboard/webapp/widgets/data_table/filter_dialog_test.ts

+    fixture.componentInstance.discreteValueFilter = 'ba';
+    fixture.detectChanges();
+    expect(
+      (await rootLoader.getAllHarnesses(MatCheckboxHarness)).length


This would still make me more confident. (You resolved this but I didn't see corresponding changes).

## Motivation for features / changes #6493 Broke our internal sync Googlers see [cl/557499468](http://cl/557499468) The issue is related to our internal Angular component migration. New components are not allowed to rely on the legacy components. Due to this the new component added in #6493 uses the non legacy checkbox. However, our dependency remapping always remaps dependencies on material_checkbox to legacy_material_checkbox. To fix this I am changing the name of the remapping rule in [cl/557515881](http://cl/557515881) and adjusting all the OSS usage EXCEPT for the usage in filter_dialog. [cl/557590954](http://cl/557590954) is also required to map the testing dependency needed to use `MatCheckboxHarness`

…6553) ## Motivation for features / changes Now that the filter dialog is merged #6493 we need to add support for filtering by hparams. However, #6544 changed the way hparam values are read. This change adds a new property to state in which to store dashboard related hparam and metric filters. See #6488 for the WIP integration with the runs_table_container + tb_data_table -> common_selectors + hparam_selectors.

## Motivation for features / changes We want to be able to filter the runs table by hparam. The existing logic for this is handled by a bunch of inner components found in the runs_table_component which makes it hard to share. In this PR I am replicating that UI with some enhancements in a standalone component which will later be used by the data_table_component. See tensorflow#6488 for screenshots and a more complete description of what is being added ## Alternate designs / implementations considered (or N/A) I'm not sure that this component should be part of the data table but that is simplest place to put it for now.

…low#6547) ## Motivation for features / changes tensorflow#6493 Broke our internal sync Googlers see [cl/557499468](http://cl/557499468) The issue is related to our internal Angular component migration. New components are not allowed to rely on the legacy components. Due to this the new component added in tensorflow#6493 uses the non legacy checkbox. However, our dependency remapping always remaps dependencies on material_checkbox to legacy_material_checkbox. To fix this I am changing the name of the remapping rule in [cl/557515881](http://cl/557515881) and adjusting all the OSS usage EXCEPT for the usage in filter_dialog. [cl/557590954](http://cl/557590954) is also required to map the testing dependency needed to use `MatCheckboxHarness`

…ensorflow#6553) ## Motivation for features / changes Now that the filter dialog is merged tensorflow#6493 we need to add support for filtering by hparams. However, tensorflow#6544 changed the way hparam values are read. This change adds a new property to state in which to store dashboard related hparam and metric filters. See tensorflow#6488 for the WIP integration with the runs_table_container + tb_data_table -> common_selectors + hparam_selectors.

rileyajones force-pushed the filter-dialogue branch from 62ac08a to f672689 Compare July 13, 2023 21:22

rileyajones commented Jul 13, 2023

View reviewed changes

rileyajones marked this pull request as ready for review July 13, 2023 22:57

rileyajones requested a review from bmd3k July 13, 2023 22:57

bmd3k reviewed Jul 14, 2023

View reviewed changes

rileyajones force-pushed the filter-dialogue branch from f672689 to 153a878 Compare July 18, 2023 16:57

rileyajones requested a review from bmd3k July 19, 2023 22:43

bmd3k reviewed Jul 21, 2023

View reviewed changes

rileyajones added 3 commits August 11, 2023 17:59

create filter dialogue component and fix existing typing error

511c21b

rename filter dialogue to filter dialog and fix license

2345d8a

change to using non legacy mat component

f83bbec

rileyajones changed the title ~~HParams: Create filter dialogue component~~ HParams: Create filter dialog component Aug 11, 2023

rileyajones added 2 commits August 11, 2023 18:09

rename files

3cfa943

address pr comments

16105dc

rileyajones force-pushed the filter-dialogue branch from 77b371d to 16105dc Compare August 11, 2023 22:08

rileyajones requested a review from bmd3k August 11, 2023 22:09

bmd3k reviewed Aug 14, 2023

View reviewed changes

make tests more rigorous as per pr comments

1e47e93

rileyajones requested a review from bmd3k August 15, 2023 17:06

bmd3k approved these changes Aug 15, 2023

View reviewed changes

rileyajones added 2 commits August 15, 2023 18:23

remove bzl file change. Test every possible filter checkbox

ded89cf

add more checks for the checkbox labels

967625a

rileyajones merged commit 4bbe464 into tensorflow:master Aug 15, 2023

rileyajones mentioned this pull request Aug 16, 2023

Bug Fix: Start using expect_angular_legacy_material_checkbox #6547

Merged

rileyajones mentioned this pull request Aug 22, 2023

HParams: Add new state property to store hparam and metric filters #6553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HParams: Create filter dialog component #6493

HParams: Create filter dialog component #6493

rileyajones commented Jul 13, 2023 •

edited

Loading

bmd3k Jul 14, 2023

bmd3k Jul 21, 2023

rileyajones Aug 11, 2023

bmd3k Jul 21, 2023

bmd3k left a comment

bmd3k Aug 14, 2023

bmd3k Aug 15, 2023

rileyajones Aug 15, 2023

bmd3k left a comment

bmd3k Aug 15, 2023

rileyajones Aug 15, 2023

bmd3k Aug 15, 2023

rileyajones Aug 15, 2023

bmd3k Aug 15, 2023

HParams: Create filter dialog component #6493

HParams: Create filter dialog component #6493

Conversation

rileyajones commented Jul 13, 2023 • edited Loading

Motivation for features / changes

Alternate designs / implementations considered (or N/A)

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

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

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

rileyajones commented Jul 13, 2023 •

edited

Loading