[DPO] Refactor eval logging of dpo trainer

[mnoukhov]

Address part 1 of #952 by removing store_metrics and switching to more standard compute_metrics letting Trainer handle logging

[lvwerra]

Thanks for working on this! Just ping me whenever you would like to run the CI.

[mnoukhov]

@lvwerra, I completed this part of the refactor, can you run the CI?

@kashif as part of this, I removed logging all the logits since it seemed a bit excessive to pass around the batch x seq_len x vocab size array and taking the mean didn't feel like a meaningful thing to log since we already have logps. Let me know if you'd like this reversed

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

[kashif]

@mnoukhov are you also logging sample outputs as well here?

[mnoukhov]

@kashif I didn't change sample output logging in this PR, so it's still there. I was going to address the sample logging issue (2 in #952) in a second PR to keep things managable

[kashif]

ok makes sense!

[kashif]

so this PR LGTM! feed free to merge

[kashif]

@mnoukhov so folks really want the additional metrics to be logged during training which isn't possible at the moment, so lets revert this for now and continue this refactoring to figure that out.

[mnoukhov]

I don't think this change prevents additional metrics from being logged. I can create a followup PR that addresses those comments

[kashif]

Ok so let's try to add the previous behavior if possible... I couldn't figure it out at first glance... I think the trainer's training loop doesn't seem to have a way to do it... can you kindly confirm?

[mnoukhov]

Basically what #1046 wants is to log metrics during training, which is not how Trainer usually works but it is a common ask e.g. https://discuss.huggingface.co/t/logging-training-accuracy-using-trainer-class/5524/3. The reverted code does not do this correctly.

AFAIK the problem with trying to put self.log into compute_loss is it doesn't really make sense whenever you use multi-gpu and it didn't account for different batch sizes. The code puts all metrics from a single gpu's minibatch into stored_metrics and then takes the mean at each self.log call. There needs to be an accelerator.gather_for_metrics somewhere in there in order to account for different batch sizes and multi-gpu. @lewtun, is this right?

If you want metrics for the training set, we can either add keep this stored_metrics and try to make it work on multi-gpu but it feels a bit ugly. The other options are to call evaluate on the training set any time it is also done on the test set, which has its own set of problems.