Hparams: Limit number of Hparams retrieved by default. #6777
Conversation
the column selector.
| @@ -35,6 +35,8 @@ import {BehaviorSubject} from 'rxjs'; | |||
| }) | |||
| export class ColumnSelectorComponent implements OnInit, AfterViewInit { | |||
| @Input() selectableColumns: ColumnHeader[] = []; | |||
| @Input() numColumnsLoaded!: number; | |||
There was a problem hiding this comment.
Why do we need this prop? Can't you just use selectableColumns.length?
There was a problem hiding this comment.
SelectableColumns does not include all columns (for example, the ones that have already been selected).
| @@ -101,10 +102,12 @@ export interface BackendHparamsExperimentResponse { | |||
|
|
|||
| interface HparamsColFilterParams { | |||
| hparam: string; | |||
| includeInResult: boolean; | |||
There was a problem hiding this comment.
Can you explain what this is doing? What is a result? Why would something not be included?
I'm kinda assuming this is a flag returned by the server which indicates that an hparam was included in the response, but that seems kinda redundant?
There was a problem hiding this comment.
api.proto probably has the best documentation for this:
I'm following the pattern of the rest of the file of not repeating the documentation here.
The api.proto documentation is not perfect, however. Left unsaid is that if you don't set includeInResult field on any of the fields in the request then the backend assumes you want all fields, even if they are not specified as a col param. I believe this is for backward compatibility.
So we have to set includeInResult or the backend would return hparam values for all hparams and not just the 1000 we retrieved in the /experiment request.
There was a problem hiding this comment.
Thanks for the link, I think I understand now.
| @@ -20,6 +20,8 @@ | |||
| [sortingInfo]="sortingInfo" | |||
| [columnFilters]="columnFilters" | |||
| [selectableColumns]="selectableColumns" | |||
| [numColumnsLoaded]="numColumnsLoaded" | |||
| [hasMoreColumnsToLoad]="numColumnsLoaded == numColumnsToLoad" | |||
There was a problem hiding this comment.
| [hasMoreColumnsToLoad]="numColumnsLoaded == numColumnsToLoad" | |
| [hasMoreColumnsToLoad]="numColumnsLoaded === numColumnsToLoad" |
| @@ -101,10 +102,12 @@ export interface BackendHparamsExperimentResponse { | |||
|
|
|||
| interface HparamsColFilterParams { | |||
| hparam: string; | |||
| includeInResult: boolean; | |||
There was a problem hiding this comment.
Thanks for the link, I think I understand now.
…d in default. (#6780) In #6777 we limited the number of hparams we load by default. Now we add the option for the user to load all hparams in spite of the performance implications. OSS Light Mode:  OSS Dark Mode:  Internal Light Mode:  Internal Dark Mode: 
Limit the number of hparams retrieved by default to 1000 for display in Time Series. Print retrieval information: # of columns loaded and whether there could be more. This is to address performance issues with loading 1000s of hparams from the backend and trying to render them all. In a future PR we will add ability to "Load All" if user wants to nonetheless load them all. Screenshot (with default set to 2 instead of 1000): 
…d in default. (tensorflow#6780) In tensorflow#6777 we limited the number of hparams we load by default. Now we add the option for the user to load all hparams in spite of the performance implications. OSS Light Mode:  OSS Dark Mode:  Internal Light Mode:  Internal Dark Mode: 
Limit the number of hparams retrieved by default to 1000 for display in Time Series. Print retrieval information: # of columns loaded and whether there could be more.
This is to address performance issues with loading 1000s of hparams from the backend and trying to render them all.
In a future PR we will add abitlity to "Load All" if user wants to nonetheless load them all.
Screenshot (with default set to 2 instead of 1000):
