Make metrics serializable #3001
Conversation
|
Shall I add |
There was a problem hiding this comment.
Thanks for the PR @sadra-barikbin !
The approach looks OK to me, let's update all metrics.
…ble' into Feature-make-metrics-serializable
|
|
||
| to_save = {'trainer': trainer, 'model': model, 'optimizer': optimizer, 'lr_scheduler': lr_scheduler} | ||
| to_save = {'trainer': trainer, 'model': model, 'optimizer': optimizer, 'lr_scheduler': lr_scheduler, 'metric': metric} |
There was a problem hiding this comment.
Ideally, we would like to get that in a more automatic way. Also if we want to save multiple metrics adding them one by one to save/load can be a pain.
Let's propose something in a follow-up PR
There was a problem hiding this comment.
Could you please reiterate what you mean? If we have multiple metrics, is it cumbersome to do:
to_save = {..., 'metric1': metric1, 'metric2': metric2, 'metric3': metric3}?
There was a problem hiding this comment.
Yes, it is too verbose, let's try to find a way to simplify that.
| @@ -87,6 +87,9 @@ def log_running_avg_metrics(): | |||
| """ | |||
|
|
|||
| required_output_keys = None | |||
| # TODO Shall we put `src` here? Then we should add a new branch for metric-typed attributes in `state_dict` | |||
| # and `load_state_dict`. Examples; This class; `Rouge` which has a `List[_BaseRouge]`. | |||
| _state_dict_all_req_keys = ("_value",) | |||
There was a problem hiding this comment.
@vfdev-5 , What's your thoughts on this?
There was a problem hiding this comment.
Let's add src and in state_dict/load_state_dict put an explicit check for isinstance(attr, Metric) and call state[attr_name].update(attr.state_dict) or something like that.
Do you have any other options ?
Fixes #2999