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

Add DS-Chat comparison #538

Merged
merged 15 commits into from
Aug 29, 2023
Merged

Add DS-Chat comparison #538

merged 15 commits into from
Aug 29, 2023

Conversation

cat-state
Copy link
Collaborator

@cat-state cat-state commented Jul 24, 2023

This PR optimizes the implementation of NeMo-PPO, adding hydra and bug fixes.

@cat-state cat-state marked this pull request as ready for review July 31, 2023 16:19
@cat-state cat-state mentioned this pull request Jul 31, 2023
@Dahoas Dahoas self-requested a review August 1, 2023 15:11
Copy link
Collaborator

@Dahoas Dahoas left a comment

Choose a reason for hiding this comment

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

Looks good overall! Just left some comments on parts I looked closely at and didn't quite understand

trlx/models/modeling_nemo_ppo.py Show resolved Hide resolved
trlx/models/modeling_nemo_ppo.py Show resolved Hide resolved
trlx/models/modeling_nemo_ppo.py Show resolved Hide resolved
trlx/models/modeling_nemo_ppo.py Show resolved Hide resolved
@Dahoas
Copy link
Collaborator

Dahoas commented Aug 28, 2023

@cat-state I'm happy to merge this today if there are no other changes from your side

@cat-state
Copy link
Collaborator Author

@cat-state I'm happy to merge this today if there are no other changes from your side

Yup, I don't have any changes on my side

@Dahoas Dahoas merged commit dad174c into main Aug 29, 2023
2 checks passed
@maxreciprocate maxreciprocate deleted the nemo-ppo-vs-ds-chat branch August 29, 2023 13:50
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.

2 participants