-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix CSVLogger hyperparameter is logged at every write which increase latency significantly. #20594
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 CSVLogger hyperparameter is logged at every write which increase latency significantly. #20594
Conversation
for more information, see https://pre-commit.ci
…ydl/pytorch-lightning into feature/csvlogger-hparams-fix
…ydl/pytorch-lightning into feature/csvlogger-hparams-fix
I search the issues and believe this would fix #19240. |
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 @duydl! Could you add a test for this please? Let us know if you need a hand 🙂
@ethanwharris Hi, for the test there is only one line needed adjustment I added in the newest commit. Now to save hyperparams with CSVLogger we no longer need to call |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20594 +/- ##
=========================================
- Coverage 88% 79% -9%
=========================================
Files 267 264 -3
Lines 23369 23311 -58
=========================================
- Hits 20478 18364 -2114
- Misses 2891 4947 +2056 |
@ethanwharris Hi, do you think this is ready to be merged? |
…latency significantly. (#20594) * Move save_hparams_to_yaml to log_hparams instead of auto save with metric * Fix params to be optional * Adjust test * Fix test_csv, test_no_name --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit 1f5add3)
…latency significantly. (#20594) * Move save_hparams_to_yaml to log_hparams instead of auto save with metric * Fix params to be optional * Adjust test * Fix test_csv, test_no_name --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit 1f5add3)
What does this PR do?
In
CSVLogger
, thesave
method of experimentExperimentWriter
is override such that hyperparameters are saved at every write, andlog_hyperparams
only update hparam, which is not efficient. The training run become very slow when there is a significantly big object in hparams and regular logging (like every hundred steps). So I believe this change is necessary.Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Reviewer checklist
📚 Documentation preview 📚: https://pytorch-lightning--20594.org.readthedocs.build/en/20594/