Skip to content

Small changes when integrating into H4#216

Merged
natolambert merged 2 commits intomainfrom
nol_nits
Mar 14, 2023
Merged

Small changes when integrating into H4#216
natolambert merged 2 commits intomainfrom
nol_nits

Conversation

@natolambert
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

HuggingFaceDocBuilderDev commented Mar 14, 2023

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

@natolambert
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
yxliu-TAMU pushed a commit to mincheolseong/ECEN743-GRPO-Project-Proposal that referenced this pull request Apr 20, 2025
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