HParams: Update the runs table to read columns from redux state #6404
Conversation
JamesHollyer
left a comment
JamesHollyer
left a comment
There was a problem hiding this comment.
The Map -> Record thing is not a big deal. The only reason I am not approving yet is the missing test in the runs_table_container.
tensorboard/webapp/runs/views/runs_table/runs_table_container.ts
Outdated
Show resolved
Hide resolved
|
|
||
| export type RunToHParamValues = Record< | ||
| string, | ||
| Record<string, string | number | boolean | undefined> |
There was a problem hiding this comment.
Can this be Record<string, HparamValue['value']> as used in the link below. Or maybe assigning that type its own name? Also, why do we need undefined? Assuming we do this could be HparamValue['value'] | undefined.
https://source.corp.google.com/piper///depot/google3/third_party/tensorboard/webapp/runs/views/runs_table/types.ts;l=44?q=project:tensorboard%20RunTableItem
I guess the way you have it is fine too....assuming you do need undefined.
There was a problem hiding this comment.
I like that idea. I'll also move this type to runs data source types then, I think that makes more sense.
| .pipe( | ||
| map((items) => { | ||
| return items.reduce((map, item) => { | ||
| map[item.run.id] = Object.fromEntries(item.hparams.entries()); |
There was a problem hiding this comment.
This seems needlessly complicated. What is wrong with just using the Map?
| }); | ||
| }); | ||
|
|
||
| describe('#getRunsTableHeaders', () => { |
There was a problem hiding this comment.
I would actually be fine without tests for simple return selectors with no logic. They are written so leave them. Maybe we can discuss this with the FE team sometime.
There was a problem hiding this comment.
That does seem like a good topic for an FE meeting. It feels like a bit of a waste of time to write them but it's been the convention to do so
rileyajones
force-pushed
the
runs-table-selectors
branch
from
d387bd6 to
5a53cc2
Compare
2 participants
Motivation for features / changes
As part the effort to add hparams to time series we have updated the runs table to use the data table in #6394. Now I am updating the runs data table to read its columns from the redux state.
Screenshots of UI changes (or N/A)
N/A (but there will be soon!)
Alternate designs / implementations considered (or N/A)
We could have reused the existing sorting property in redux and added keys to it, but I prefered to create a seperate one and remove the existing property when we remove the old runs table.