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

Small changes when integrating into H4 #216

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Small changes when integrating into H4 #216

merged 2 commits into from
Mar 14, 2023

Conversation

natolambert
Copy link
Contributor

@natolambert natolambert commented Mar 14, 2023

Two changes:

  1. Pass the optimizer in the sentiment example (currently variable was not passed into trainier).
  2. [I think] fix the kwarg option for wandb config of Accelerate. See this docs page, where init_kwargs is handled differently. In trying to use this with the code as is, wandb is getting read as a kwarg and not handled correctly by this line. If this is different in Tensorboard, it may just be incompatible.

Let me know if I'm wrong!

Fixes: #215

@natolambert
Copy link
Contributor Author

Closes ##215 if correct on point 1 @younesbelkada !

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 14, 2023

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

@natolambert
Copy link
Contributor Author

I tested the logging change with my code in H4 #https://github.com/huggingface/h4/pull/73, and it fixed my problem!

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 the PR
Agreed for the first point! Great catch!
Regarding the second point I have slight doubts that it may break things with tensorboard, if this is not vital we can leave it on a follow up PR to test it properly - otherwise you can quickly test any script with log_with="tensorboard" and see if the training runs
Thanks!

@natolambert
Copy link
Contributor Author

I'll test tensorboard today. FYI this is needed for the script in H4, so I'll be motivated to get this working soon.

If tensorboard doesn't work, I'll prolly do an if statement.

@natolambert
Copy link
Contributor Author

@younesbelkada I think I ran this with tensorboard (just changed the config to as follows and it didn't error). Seems good to me?

The term I changed tracker_kwargs was not used in any of TRL to date actually.

config = PPOConfig(
    model_name="ybelkada/gpt-j-6b-sharded-bf16",
    learning_rate=(1.47e-5) * 2,
    # log_with="wandb",
    log_with="tensorboard",
    accelerator_kwargs={"logging_dir": '/home/nathan/logs/'},
    batch_size=32,
    forward_batch_size=1,
)

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 fixing! 🔥

@younesbelkada
Copy link
Contributor

Thanks a lot for experimenting @natolambert ! LGTM

@natolambert natolambert merged commit 357730f into main Mar 14, 2023
@natolambert natolambert deleted the nol_nits branch March 14, 2023 21:15
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.

Extra code in toxicity example
3 participants