Hparams: Add include_in_result field and implement support for hparams_to_include
#6559
Conversation
| if all( | ||
| not col_param.HasField("include_in_result") for col_param in col_params | ||
| ): | ||
| return None |
There was a problem hiding this comment.
This pattern of calling a "get" method then returning None to indicate that all should be included is quite confusing. Can we just return all the params? That should reduce amount of places the code needs to care about this and minimize the number of logical branches.
| return None | |
| return [col_param.hparam for col_param in col_params] |
I would also be okay with renaming the function, or returning something more meaningful than None.
There was a problem hiding this comment.
Thanks for the super fast review! This is a good point, my only concern is if there are a large number of Hparams, then we will be passing all of them to the backend and not sure if it will worsen the performance, I'll see if I could do some testing internally later.
There was a problem hiding this comment.
@rileyajones I added @bmd3k's helper method _specifies_include() (bmd3k@ab75cd1) so that the get method is cleaner but we can still do a quick check to avoid sending all the hparams when no include field is specified.
There was a problem hiding this comment.
The internal tests are not passing, will fix this PR and let you know once it's ready.
There was a problem hiding this comment.
Actually tests passed after removing other changes outside this PR (cl/563163418).
yatbear
changed the title
include_in_result field and implement support for hparams_to_include.include_in_result field and implement support for hparams_to_include
This PR extracts the filtering logic for hparams only (metrics filtering will be done later) from bmd3k's commit.
Googlers, see comments in cl/559852837 for more context.
Test internally at cl/563163418.
#hparams