-
Notifications
You must be signed in to change notification settings - Fork 19.6k
Add verbose logging when ModelCheckpoint callback is done saving ... #21805
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
base: master
Are you sure you want to change the base?
Conversation
…del. This is useful for collecting information on how long model checkpointing takes, together with existing verbose logging on initiating the save.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @kathyfan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds a verbose log message that is displayed after a model checkpoint is saved. This is a useful addition for monitoring and timing the checkpointing process. My review identifies one issue with the implementation where the new "finished saving" log message is not always paired with a "starting to save" message, leading to inconsistent output in certain edge cases. I've left a comment with details on the issue and a suggestion for how to resolve it to ensure the logging behavior is consistent and intuitive for users.
| if self.verbose > 0: | ||
| io_utils.print_msg( | ||
| f"\nEpoch {epoch + 1}: finished saving model to {filepath}" | ||
| ) |
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.
This new log message is useful for timing the save operation. However, in its current form, it can lead to inconsistent logging output.
The "finished saving" message will be printed whenever a model is saved and verbose > 0. However, the corresponding "starting to save" message is not always printed under the same conditions.
Specifically, in _should_save_model, when save_best_only=True and the monitor metric is unavailable or not a scalar, the method returns True (triggering a save) but only issues a warning, not a verbose log message. This results in an "end" log without a "start" log, which could be confusing for users.
To ensure consistent logging, a "starting to save" message should also be logged in these fallback cases within _should_save_model. This would ensure that every "finished saving" log is paired with a "saving" log when verbose > 0.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21805 +/- ##
==========================================
+ Coverage 82.63% 82.66% +0.02%
==========================================
Files 577 577
Lines 59318 59421 +103
Branches 9300 9314 +14
==========================================
+ Hits 49019 49121 +102
+ Misses 7911 7899 -12
- Partials 2388 2401 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Updated formatting for line length.
... a model.
This is useful for collecting information on how long model checkpointing takes, together with existing verbose logging on initiating the save. This is similar to the "calling checkpoint listeners before/after saving checkpoint xxx" logging that existed in the TF1 session run hook.