Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor rlhf #328

Merged
merged 59 commits into from
Aug 15, 2023
Merged

Refactor rlhf #328

merged 59 commits into from
Aug 15, 2023

Conversation

maxjeblick
Copy link
Contributor

This PR adds a separate problem type for RLHF.

Some discussion items:

  • How to structure the training settings configuration. The current order may not be optimal.
  • train.py contains two training functions (run_train_rlhf and run_train) with partially duplicated code. IMO, it is ok to keep it as is rather than having one function with multiple if-else statements.
  • Train data insights should be redone (Target Text is not used, thus always an empty string). Will implement it here after Max/insights table view #301 has been merged.

Fixes #317
We can first merge #308 and I will fix potential merge conflicts subsequently.

@maxjeblick maxjeblick marked this pull request as draft August 7, 2023 10:10
@maxjeblick maxjeblick marked this pull request as ready for review August 7, 2023 19:46
@maxjeblick
Copy link
Contributor Author

Should be good for a first review.

Some points I noticed:

  • Train data insights do not display the answer text. We could either leave it as is (highlighting that training is not using ground truth answer), or removing the field by subclassing the plot class.
  • Some inference parameters for RLHF are hidden in the chat window. One could explicitly change the visibility in the chat tab?

Copy link
Collaborator

@pascal-pfeiffer pascal-pfeiffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @maxjeblick .
I really like your refactors and it makes great sense to put RLHF in an own problem type.

I will still need to go through the model and train changes and also do some local testing, but will already provide you some initial thoughts now.

Copy link
Collaborator

@pascal-pfeiffer pascal-pfeiffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

  • LLM Backbone is not a dropdown for me

  • hide output dir

We could probably move the reward model to the top, too, wdyt?

@pascal-pfeiffer
Copy link
Collaborator

and let's also remove

  • FSDP
  • skip parent proba
  • random parent proba

@maxjeblick
Copy link
Contributor Author

Thanks for the review, it was very helpful. I addressed the issues above, in addition I added some smaller code changes:

  • I fixed a small bug in plot_batch
  • I refactored .from_dict class method to a common base method.

We could probably move the reward model to the top, too, wdyt?

Yes good idea, I changed that.

Copy link
Collaborator

@pascal-pfeiffer pascal-pfeiffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @maxjeblick , very nice refactor and small fixes along the way!
Looks all good to me now.

I will likely work on RLHF again in a follow up PR/issue and allow for larger batches. It should be much easier now with the individual train loops.

@maxjeblick maxjeblick merged commit be29c33 into main Aug 15, 2023
5 checks passed
@maxjeblick maxjeblick deleted the max/refactor_rlhf branch August 15, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CODE IMPROVEMENT] Promote RLHF as a separate problem type
2 participants