-
Notifications
You must be signed in to change notification settings - Fork 25.6k
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
Trainer + wandb quality of life logging tweaks #6241
Conversation
…th trainer arguments
Codecov Report
@@ Coverage Diff @@
## master #6241 +/- ##
==========================================
- Coverage 79.61% 79.57% -0.04%
==========================================
Files 146 146
Lines 26597 26600 +3
==========================================
- Hits 21175 21167 -8
- Misses 5422 5433 +11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! Could you copy the lines in setup for TFTrainer
as well and the doc line in TFTrainingArguments
? (no need to redefine it there with field as TFTrainingArguments
subclasses TFTrainingArguments
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Updated the PR with your comments! |
But CI looks like it broke inbetween |
I love the fact that more parameters are now auto logged. For info the approach right now in this integration was to pass environment variables to configure You can also call People use a lot of different init parameters so you can either add them manually (like with the run_name one) or could use a "kwargs" approach to accept more (and extract all the valid ones or even just the ones starting with |
Hello @LysandreJik and friends! If you like more items being logged, hopefully someone can review #6176 sooner than later to cut down on merge conflicts. I re-arranged some of the wandb and tensorboard initialization code to better accommodate what could be a growing list of integrations. |
@LysandreJik @dsblank do you think we should merge #6176 first? |
Merging this right now since it's ready. @dsblank Sorry for the delay in reviewing your PR, I'm heading there next and will help solve potential merge conflicts if needed. |
As discussed on Slack, this PR adds the possibility for users to specify a
name
for the run in their wandb project, and logs the model config in addition to the trainer args in thewandb.init
call (if there is a duplicate key, it is overriden by the trainer args)