Conversation
|
Shall I add |
vfdev-5
left a comment
vfdev-5
left a comment
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
| metric = ... | ||
|
|
||
| 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.
vfdev-5
left a comment
vfdev-5
left a comment
There was a problem hiding this comment.
LGTM, thanks @sadra-barikbin
| 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 ?
2 participants
Fixes #2999