policy kl [old | new]

[kashif]

I believe the kl we want to record is KL[policy_old || policy_new]

Perhaps @natolambert can confirm?

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[natolambert]

For things like this, I think it's best to run a couple examples with and without the change. In theory, I think you can be right (@lvwerra probably knows).

I went back to the original repo and saw that TRL matches it.

https://github.com/openai/lm-human-preferences/blob/bd3775f200676e7c9ed438c50727e7452b1a52c1/lm_human_preferences/train_policy.py#L360

[kashif]

thanks @natolambert yes the approxkl is fine since it's the square of the difference so the order is not important. It's the policykl which is not correct i believe.

[lvwerra]

I wonder where that actually came from since it was not part of the original implementation 😂. True, if policykl should measure the KL-div of the current to the old policy then it's the wrong way round. Since it's only used for monitoring I am up to changing it unless there is a strong incentive to keep it that way.

[kashif]

yes in any case it's just for logging, it's not used to back-prop etc. I check other implementations e.g. ray, spinningup etc. and they have it as in this fix.