-
Notifications
You must be signed in to change notification settings - Fork 517
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
Fix WandBLogger to allow resuming runs with updated config values #1081
Fix WandBLogger to allow resuming runs with updated config values #1081
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1081
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 8063ffc with merge base 74fb5e4 (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Hi @parthsarthi03! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Thanks @parthsarthi03 for the PR! The change looks fine to me, but lint is failing. Mind fixing locally? Once CI is green I can merge this |
Done! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1081 +/- ##
==========================================
- Coverage 26.66% 26.65% -0.01%
==========================================
Files 183 183
Lines 8326 8329 +3
==========================================
Hits 2220 2220
- Misses 6106 6109 +3 ☔ View full report in Codecov by Sentry. |
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 the fix! CI is green, so will merge this in
Context
This PR aims to:
It addresses the issue #1080 where the
WandBLogger
fails to resume a run with an updated config file whenallow_val_change
is set toTrue
in the config.Changelog
The changes made in this PR are:
self.config_allow_val_change = kwargs.get('allow_val_change', False)
in the__init__
method of theWandBLogger
class to retrieve the value ofallow_val_change
from thekwargs
dictionary and store it as an instance variable.self._wandb.config.update(resolved)
toself._wandb.config.update(resolved, allow_val_change=self.config_allow_val_change)
in thelog_config
method to pass theallow_val_change
option to theupdate
method based on the value provided during the initialization of theWandBLogger
.Test plan
Tested the modified
WandBLogger
by resuming a run with an updated config file whereallow_val_change
was set toTrue
. The run resumed successfully without any errors, and the config values were updated as expected.