-
Notifications
You must be signed in to change notification settings - Fork 19.6k
avoid failure when docstrings have been stripped (python -OO) #21477
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
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.
Summary of Changes
Hello @JohannesBuchner, 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 robustness of the Keras library by preventing runtime errors that occur when Python docstrings are stripped, such as when running Python with the -OO
optimization flag. It ensures that internal docstring manipulation logic gracefully handles the absence of docstrings, improving the library's compatibility and stability in optimized environments.
Highlights
- Docstring Robustness: Implemented checks to ensure that operations on
__doc__
attributes (like concatenation or string replacement) do not fail when docstrings have been stripped (e.g., by running Python with the-OO
flag). This involves either conditionally checking if__doc__
isNone
or providing a default empty string.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
tensorflow/tensorflow#96906 needs to be applied as well (if using tensorflow) |
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 addresses an important issue where docstring manipulation could fail when running Python with the -OO
flag. The changes correctly prevent crashes by checking if __doc__
is None
.
keras/src/applications/densenet.py
Outdated
setattr(DenseNet121, "__doc__", (DenseNet121.__doc__ or '') + DOC) | ||
setattr(DenseNet169, "__doc__", (DenseNet169.__doc__ or '') + DOC) | ||
setattr(DenseNet201, "__doc__", (DenseNet201.__doc__ or '') + DOC) |
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.
The current implementation uses ... or ''
which can add an empty docstring even when docstrings are intended to be stripped (e.g., with python -OO
). This differs from the approach in other files.
To ensure consistency and improve code clarity, consider using a conditional check within a loop. This aligns the behavior with the rest of the changes in this pull request and enhances readability.
setattr(DenseNet121, "__doc__", (DenseNet121.__doc__ or '') + DOC) | |
setattr(DenseNet169, "__doc__", (DenseNet169.__doc__ or '') + DOC) | |
setattr(DenseNet201, "__doc__", (DenseNet201.__doc__ or '') + DOC) | |
models = [DenseNet121, DenseNet169, DenseNet201] | |
for model in models: | |
if model.__doc__ is not None: | |
setattr(model, "__doc__", model.__doc__ + DOC) |
if ResNet50.__doc__ is not None: | ||
setattr(ResNet50, "__doc__", ResNet50.__doc__ + DOC) | ||
setattr(ResNet101, "__doc__", ResNet101.__doc__ + DOC) | ||
setattr(ResNet152, "__doc__", ResNet152.__doc__ + DOC) |
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.
The current check if ResNet50.__doc__ is not None:
assumes that if ResNet50
has a docstring, then ResNet101
and ResNet152
will as well. While this might be true now, it's better to check each model's docstring individually for robustness and to avoid potential issues if new model variants are added without docstrings in the future.
if ResNet50.__doc__ is not None: | |
setattr(ResNet50, "__doc__", ResNet50.__doc__ + DOC) | |
setattr(ResNet101, "__doc__", ResNet101.__doc__ + DOC) | |
setattr(ResNet152, "__doc__", ResNet152.__doc__ + DOC) | |
models = [ResNet50, ResNet101, ResNet152] | |
for model in models: | |
if model.__doc__ is not None: | |
setattr(model, "__doc__", model.__doc__ + DOC) |
if ResNet50V2.__doc__ is not None: | ||
setattr(ResNet50V2, "__doc__", ResNet50V2.__doc__ + DOC) | ||
setattr(ResNet101V2, "__doc__", ResNet101V2.__doc__ + DOC) | ||
setattr(ResNet152V2, "__doc__", ResNet152V2.__doc__ + DOC) |
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.
Similar to resnet.py
, the check if ResNet50V2.__doc__ is not None:
assumes that if ResNet50V2
has a docstring, then ResNet101V2
and ResNet152V2
will too. To make the code more robust and easier to maintain, it's better to check each model's docstring individually.
if ResNet50V2.__doc__ is not None: | |
setattr(ResNet50V2, "__doc__", ResNet50V2.__doc__ + DOC) | |
setattr(ResNet101V2, "__doc__", ResNet101V2.__doc__ + DOC) | |
setattr(ResNet152V2, "__doc__", ResNet152V2.__doc__ + DOC) | |
models = [ResNet50V2, ResNet101V2, ResNet152V2] | |
for model in models: | |
if model.__doc__ is not None: | |
setattr(model, "__doc__", model.__doc__ + DOC) |
if ResNet50.__doc__ is not None: | ||
setattr(ResNet50, "__doc__", ResNet50.__doc__ + DOC) | ||
setattr(ResNet101, "__doc__", ResNet101.__doc__ + DOC) | ||
setattr(ResNet152, "__doc__", ResNet152.__doc__ + DOC) |
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.
Adding a conditional check ensures that the docstring replacement only occurs if the ResNet50.__doc__
attribute is not None. This prevents potential errors when docstrings are stripped.
if ResNet50.__doc__ is not None: | |
setattr(ResNet50, "__doc__", ResNet50.__doc__ + DOC) | |
setattr(ResNet101, "__doc__", ResNet101.__doc__ + DOC) | |
setattr(ResNet152, "__doc__", ResNet152.__doc__ + DOC) | |
if ResNet50.__doc__ is not None: | |
setattr(ResNet50, "__doc__", ResNet50.__doc__ + DOC) | |
setattr(ResNet101, "__doc__", ResNet101.__doc__ + DOC) | |
setattr(ResNet152, "__doc__", ResNet152.__doc__ + DOC) |
if ResNet50V2.__doc__ is not None: | ||
setattr(ResNet50V2, "__doc__", ResNet50V2.__doc__ + DOC) | ||
setattr(ResNet101V2, "__doc__", ResNet101V2.__doc__ + DOC) | ||
setattr(ResNet152V2, "__doc__", ResNet152V2.__doc__ + DOC) |
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.
Adding a conditional check ensures that the docstring replacement only occurs if the ResNet50V2.__doc__
attribute is not None. This prevents potential errors when docstrings are stripped.
if ResNet50V2.__doc__ is not None: | |
setattr(ResNet50V2, "__doc__", ResNet50V2.__doc__ + DOC) | |
setattr(ResNet101V2, "__doc__", ResNet101V2.__doc__ + DOC) | |
setattr(ResNet152V2, "__doc__", ResNet152V2.__doc__ + DOC) | |
if ResNet50V2.__doc__ is not None: | |
setattr(ResNet50V2, "__doc__", ResNet50V2.__doc__ + DOC) | |
setattr(ResNet101V2, "__doc__", ResNet101V2.__doc__ + DOC) | |
setattr(ResNet152V2, "__doc__", ResNet152V2.__doc__ + DOC) |
keras/src/applications/densenet.py
Outdated
setattr(DenseNet121, "__doc__", (DenseNet121.__doc__ or '') + DOC) | ||
setattr(DenseNet169, "__doc__", (DenseNet169.__doc__ or '') + DOC) | ||
setattr(DenseNet201, "__doc__", (DenseNet201.__doc__ or '') + DOC) |
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.
Adding a conditional check ensures that the docstring replacement only occurs if the DenseNet121.__doc__
attribute is not None. This prevents potential errors when docstrings are stripped.
setattr(DenseNet121, "__doc__", (DenseNet121.__doc__ or '') + DOC)
setattr(DenseNet169, "__doc__", (DenseNet169.__doc__ or '') + DOC)
setattr(DenseNet201, "__doc__", (DenseNet201.__doc__ or '') + DOC)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21477 +/- ##
===========================================
- Coverage 82.80% 61.97% -20.83%
===========================================
Files 565 565
Lines 55505 55590 +85
Branches 8662 8688 +26
===========================================
- Hits 45962 34453 -11509
- Misses 7429 19001 +11572
- Partials 2114 2136 +22
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:
|
Addresses #21476