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

PPO config __init__ is bloated #241

Merged
merged 7 commits into from
Mar 24, 2023

Conversation

GauravVirmani
Copy link
Contributor

What does this PR do?

Fixes: #233

Moving total_ppo_epochs, forward_batch_size and log_with to post init method and let the dataclass automatically assign the other member variables.

cc @edbeeching @lvwerra

gaurav.vi added 2 commits March 22, 2023 20:10
… init method and let the dataclass automatically assign the other member variables.
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 22, 2023

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

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Awesome work, that looks great to me. Thanks a mile for fixing

@edbeeching
Copy link
Collaborator

edbeeching commented Mar 23, 2023

This is great, that your for addressing this.
Would it be worthwhile removing the long args description and using fields + metadata?
For example: log_with: Optional[str] = field(default=None, metadata={"help": "use 'wandb' to log with wandb"})

@younesbelkada
Copy link
Contributor

younesbelkada commented Mar 23, 2023

Hi @GauravVirmani ,
Can you run the styling checks?

make style && make quality

@younesbelkada
Copy link
Contributor

Hi @GauravVirmani
I think there is a mismatch between our black versions, mine is:

>>> import black
>>> black.__version__
'23.1.0'

Can you please double check?

@GauravVirmani
Copy link
Contributor Author

Hey @younesbelkada

My version is same:

image

Is there a way to run these checks in my local machine? Let me try running the styling checks again.

@younesbelkada
Copy link
Contributor

Hmm in my experience you just need to run make style && make quality. Otherwise I can do it for you

Copy link
Contributor

@younesbelkada younesbelkada 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 for all your work on this!

@GauravVirmani
Copy link
Contributor Author

Hi @younesbelkada

I have added changes to incorporate the 2 new arguments(early_stopping and target_kl) added in PR #238 to resolve merge conflict with main branch.

@edbeeching @lvwerra It would be great if you can review and get this merged.

@lvwerra
Copy link
Member

lvwerra commented Mar 24, 2023

Thanks @GauravVirmani for fixing the merged PR!

@lvwerra lvwerra merged commit 9523474 into huggingface:main Mar 24, 2023
@GauravVirmani GauravVirmani deleted the ppo-config-post-init branch March 24, 2023 09:38
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.

PPO config __init__ is bloated
5 participants