Improvements 1a #70
Improvements 1a #70
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
edbeeching
force-pushed
the
improvements-1a
branch
from
1fb877a to
5286000
Compare
edbeeching
force-pushed
the
improvements-1a
branch
from
abaf0e4 to
f3c9ab6
Compare
There was a problem hiding this comment.
Thanks @edbeeching, this looks great! Left a few minor comments, can also address some of them in a follow up PR.
| # define a reward for response | ||
| # (this could be any reward such as human feedback or output from another model) | ||
| reward = [torch.tensor(1.0)] | ||
| reward = [torch.tensor(1.0).to(device)] |
There was a problem hiding this comment.
We should let accelerate handle device placement internally. (Can address it in a follow up PR if you want)
There was a problem hiding this comment.
I think the reward would normally be from a preference model, so it would probably be on the correct device. But in this example that is not the case. I'll leave it to another PR.
There was a problem hiding this comment.
yes, discussed with @younesbelkada that we add some device placement steps to PPOTrainer.step.
There was a problem hiding this comment.
yes, this will be addressed in a PR right after merging this
| # isort: off | ||
| from .utils import AdaptiveKLController, FixedKLController | ||
|
|
||
| # isort: on | ||
|
|
There was a problem hiding this comment.
Why are the utils imports separate from the others?
There was a problem hiding this comment.
This causes a circular import in PPOTrainer if we allow isort to sort the imports. I added a comment. Alternatively, we could exclude these from the init.py file and/or use a relative import in PPOTrainer.
This PR: