compute_metrics in the trainer does not seem to be extensible 
Author: rabeehkarimimahabadi
Pull request: no

Hi,

This is more feature request, looking into compute_metrics function defined below:
https://github.com/huggingface/transformers/blob/c89bdfbe720bc8f41c7dc6db5473a2cb0955f224/src/transformers/trainer.py#L204

to me this looks like the design does not allow user easy modification for different applications or I am missing something, please find the explanations below:

Lets assume the user has multiple tasks like in T5 and each task needs multiple different evaluation metrics, which needs to be generated on the fly, then since this function does not accept any more arguments than `EvalPrediction`, this would not allow user to pass further parameters to generate the final evaluation metric on the fly, I appreciate modifying the design to allow easy modifications.

thanks 

Comments:
If I understand correctly you raise 2 unrelated issues:

1. there is only one place where `compute_metrics` is set and perhaps it needs to be changed through the life of the trainer object?

Since you can always override it with:
```
trainer.compute_metrics = new_compute_metrics
```
when you need to switch it to another version, so in a pinch you can do that. But clearly this is not a public API at the moment and can change at any time.

Perhaps all is needed is a setable accessor for the `compute_metrics` attribute, so that a user can use it to swap in a new function at will, rather than adding new arguments?

2. You're saying that users may need to pass more args to `compute_metrics`,  but it's not possible.

You can do that via a closure mechanism, e.g. how it's done here:
https://github.com/huggingface/transformers/blob/cbe63949d76efd153a1f389f38fe9ce1287e06b0/examples/seq2seq/utils.py#L80
so you build your `compute_metrics` on the fly, getting whatever data you need into the closure function and then it'll have access to whatever other data you may need at run time.

Here is a silly example:
```
def make_compute_metrics():
    extra_input = 1
    def compute_metrics(pred):
        print(f"Look ma, I can pass my own args: {extra_input}")
    return compute_metrics

trainer.compute_metrics = make_compute_metrics()

# and then some time later in `prediction_loop`:
self.compute_metrics(EvalPrediction(predictions=preds, label_ids=label_ids))
# calls your created on the fly function with whatever other data you want to be seen from it.
```

So your custom `compute_metrics_fn` now can access whatever other data you want besides the `EvalPrediction` object.





Hi there
thank you for the response, yes, I agree this is possible, I solved this
with `functools.partial`, but still I think the better design would be to
allow the user add extra parameters. so this was more feature request.
Please feel free to ignore if this does not make sense.
thanks
Best
Rabeeh


On Tue, Dec 22, 2020 at 11:12 PM Stas Bekman <notifications@github.com>
wrote:

> If I understand correctly you raise 2 unrelated issues:
>
>    1. there is only one place where compute_metrics is set and perhaps it
>    needs to be changed through the life of the trainer object?
>
> Since you can always override it with:
>
> trainer.compute_metrics = new_compute_metrics
>
> when you need to switch it to another version, so in a pinch you can do
> that.
>
> Perhaps all is needed is an setable accessor for the compute_metrics
> attribute, so that a user can use to swap in a new function at will, rather
> than adding new arguments?
>
>    1. You're saying that users may need to pass more args to
>    compute_metrics, you can do that via closure, e.g. how it's done here:
>
>    https://github.com/huggingface/transformers/blob/cbe63949d76efd153a1f389f38fe9ce1287e06b0/examples/seq2seq/utils.py#L80
>    so you build your compute_metrics on the fly, getting whatever data
>    you need into the closure function and then it'll have access to whatever
>    other data you may need at run time.
>
> Here is a silly example:
>
> def make_compute_metrics():
>     extra_input = 1
>     def compute_metrics(pred):
>         print(f"Look ma, I can pass my own args: {extra_input}")
>     return compute_metrics
>
> compute_metrics_fn = make_compute_metrics()
> compute_metrics_fn()
>
> So your custom compute_metrics_fn now can access whatever other data you
> want besides the EvalPrediction object.
>
> As I have shown in (1) you can now assign this to trainer.compute_metrics.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/transformers/issues/9264#issuecomment-749827774>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ARPXHH2C7MLFOY6V3MWST5LSWERUDANCNFSM4VGBUKSQ>
> .
>


Yes, `partial` would do the trick.

I've just shared my take on it. and that there might be a need for a public API to override `trainer.compute_metrics` post-`__init__`,  

In my limited experience `partial` or a manual closure is how some projects implement such functions.

I will let others comment though on this feature request.

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md) are likely to be ignored.