HParam UI: Change SelectedStepRunData to use name #6362
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rileyajones
reviewed
May 4, 2023
Comment on lines
134
to
135
| export type SelectedStepRunData = { | ||
| [key in ColumnHeaderType]?: string | number; | ||
| } & {id: string}; | ||
| id: string; | ||
| data: TableDataPoint[]; | ||
| }; |
Contributor
There was a problem hiding this comment.
I think this list makes a lot of your other code a lot more complicated. Could this just be a record?
Suggested change
| export type SelectedStepRunData = { | |
| [key in ColumnHeaderType]?: string | number; | |
| } & {id: string}; | |
| id: string; | |
| data: TableDataPoint[]; | |
| }; | |
| export type SelectedStepRunData = {id: string} & Record<string, string|number>; |
JamesHollyer
force-pushed
the
DisambiguateDataBasedOnName
branch
5 times, most recently
from
da69fe1 to
67b6aa0
Compare
The goals is to allow for dynamic HParam columns in the data table. This
means that we cannot have an enum like ColumnHeaderType be the unique
identifier for the column headers because we cannot encapsulate all
possible hparams into an enum.
This change adds a name and displayName field to the ColumnHeader
interface. The goal here is to make name be the unique identifier for
columns. However, this PR does not use these fields at all. It simply
adds them. THERE IS NO FUNCTIONAL CHANGE in this PR.
Upcoming changes will switch the code over to start using the name as
the unique identifier and display the displayName.
(For POC to see how this change fits in googlers see cl/526767686)
N/A
POC works
There were many alternatives considered. In fact I do not believe this
is going to be the final structure.
One option is to remove the type and add a category field
export interface ColumnHeader {
name: string;
displayName: string;
Category: ColumnCategory(STATIC, HPARAM, METRIC)
enabled: boolean;
}
However, deciding how to calculate the value based on hardcoded name
strings does not seem ideal.
Another option is to have a hierarchy. I expect it to look like this:
Interface ColumnHeaderBase {
name: string;
displayName: string;
type: ColumnHeaderType;
enabled: boolean;
}
export interface StaticColumnHeader extends ColumnHeaderBase{
category: “STATIC”
}
export interface HParamColumnHeader extends ColumnHeaderBase{
category: “HParam”
source: enum(data, config,....);
differs:boolean;
}
ColumnHeader = StaticColumnHeader | HParamColumnHeader
This may be the final form that we go with however, we are not sure the
source and differs attributes need to go here or somewhere else yet. All
code which works with the type defined in this PR will work with this
proposal as well.
Lastly, there is the option which is the same as the hierarchy option
above but with StaticColumnHeader being the only interface with a "type"
attribute. This is a valid possibility but, I have decided that it just
adds unnecessary complexity by requiring a type check in many more
places.
JamesHollyer
force-pushed
the
DisambiguateDataBasedOnName
branch
from
67b6aa0 to
11190f6
Compare
rileyajones
approved these changes
May 10, 2023
JamesHollyer
added a commit
that referenced
this pull request
May 22, 2023
## Motivation for features / changes For the HParam work that is being done we are now using the name field as the unique identifier for columns. Therefore the SortingInfo interface needs to use name and not header. However, making the hard swap to using name would break sync so it is happening in stages. The name switch to using names happened in #6362. However, we had to make name optional for sync reasons. Googlers see cl/534097086 to see where name was added. Now this PR makes header optional. I will then make an internal change to remove it. Then I can finally submit a final PR which removes the property entirely.
JamesHollyer
added a commit
that referenced
this pull request
Aug 2, 2023
## Motivation for features / changes This is the final change in a long line of changes to move to using name. The original and functional change is #6362. This had to add name as an optional parameter only in order to avoid internal breakages. Now that we had name we could add the use of name internally(Googlers see cl/534191646) . Then we could switch to make the header optional and the name required which was done here: #6400. After header was optional we removed the use of headers internally(googlers see cl/552643471). Finally in this PR we removed the header attribute.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for features / changes
The eventual goal is to allow dynamic HParam columns in the data table. To do this we introduced the name and displayName attributes to the ColumnHeader in #6346. The name attribute is now going to be the unique identifier for a column instead of the type enum.
Technical description of changes
The SelectedStepRunData is used to hold the data for the table for a specific run. Previously this was an object with an optional property for all values of the ColumnHeaderType enum. This will no longer work as these values need to be more dynamic to allow for all possible Hparam values. This PR changes that object to add an array which holds all the data points associated with the name of the column which they correspond to.
Unfortunately this breaks the sorting as the array of SelectedStepRunData which is passed to the DataTable is sorted based on which header type the user chose to sort by. So that had to be updated to use the name of the column which the user chose to sort by.
Alternate designs / implementations considered (or N/A)
I thought about adding a new field to the SelectedStepRunData object called HParam value and keeping all the optional attributes. This was a big headache for ordering and sorting.