-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Remove Keras 2 guides from rendering #2259
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
Conversation
Summary of ChangesHello @sachinprasadhs, 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 focuses on streamlining the documentation and examples by removing Keras 2 specific content from the rendering process. It achieves this by systematically tagging Keras 2 examples and modifying the content generation script to filter them out, ensuring that only Keras 3 relevant guides are displayed. This change helps maintain a consistent and up-to-date resource for users. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 updates the documentation generation scripts to filter out Keras 2 examples from the examples landing page. This is achieved by adding a keras_2 flag to the relevant examples in scripts/examples_master.py and then modifying scripts/autogen.py to skip these examples during rendering. The changes also include logic to handle categories and subcategories that become empty after filtering.
The implementation is mostly correct and achieves the intended goal. I've left one comment in scripts/autogen.py with a suggestion to simplify a part of the code for better readability and maintainability.
jeffcarp
left a comment
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! Have we confirmed that all of the Keras 2 guides are not able to be updated to Keras 3? If we're not certain, it might be better to add a warning at the top of the Keras 2 guides until we can confirm. Also re: Fabien's point, I think we should keep rendering the pages, but maybe just remove them from the navigation?
| { | ||
| "path": example_path, | ||
| "title": title.strip(), | ||
| "keras_3": True, |
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.
Instead of separate keys for keras_2 and keras_3, it feels cleaner to have a keras_version key that can be 2 or 3
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 reason I introduced Keras_2 is, we already have a Keras_3 key, based on that the examples are loading the information like this https://keras.io/examples/nlp/multimodal_entailment/
Assuming this Keras_2 key is only temporary, till we figure out the fate of each examples, either to remove it or migrate it.
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.
Ah gotcha, if a guide is not keras_3 does that mean 100% the time it's Keras 2? In that case do we even need a new key?
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.
Is this still necessary given the most recent changes? Logically it seems like we only need the keras_2 key.
|
Thanks for the review. When you mentioned remove from navigation, you mean to remove from here https://keras.io/examples/? |
|
Sorry for the delay - ah I see, so it looks like we already automatically apply a warning to Keras 2 guides: Line 468 in f4ebbf6
In that case, expanding on the warning sounds good. How about something like: if version == 2:
md_content_lines.insert(
... (
"Warning: This example uses Keras 2 and may not be compatible with the latest version of Keras. "
"Please check out all of our <a href="https://keras.io/examples/">Keras 3 examples here</a>."
)
Would it be possible to decouple this so the pages still render but they don't show up in the navigation? That sounds like a smoother step forward. |
jeffcarp
left a comment
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 Sachin! Looks good, just left a comment about potential simplification.
| { | ||
| "path": example_path, | ||
| "title": title.strip(), | ||
| "keras_3": True, |
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.
Is this still necessary given the most recent changes? Logically it seems like we only need the keras_2 key.
|
This block is necessary as a fallback mechanism. If a new example is added to keras.io but isn't explicitly updated in examples_master.py, this script ensures it is still pulled from the src directory and listed under the Others category. By setting |
jeffcarp
left a comment
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.
Got it - thanks for the clarification. Left one more comment
scripts/autogen.py
Outdated
| if version == 2: | ||
| md_content_lines.insert( | ||
| i + 1, | ||
| "<div class='example_keras2_warning'><strong>Warning:</strong> This example uses Keras 2 and may not be compatible with the latest version of Keras. Please check out all of our <a href=\"https://keras.io/examples/\">Keras 3 examples here</a>.</div>", |
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.
I just realized won't this be duplicative with the warning above? I.e. it will show:
This example uses Keras 2
Warning: This example uses Keras 2 and
Can you make it so that there's only one message for 3 and one message for 2?
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.
There is no extra message for Keras 3, this warning message is only for Keras 2 tutorials.
Shall I rephrase the warning message like
"Warning: This example is legacy and may be removed in future updates. It is no longer guaranteed to function correctly. Please refer to our Keras 3 examples here"
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.
Right, but for Keras 2 there are still two separate messages, right? Can you restructure so there's only one message for both? Something like:
if version == 3:
md_content_lines.insert( {keras 3 message} )
elif version == 2:
md_content_lines.insert( {keras 2 message} )
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.
Just to clarify.
Currently this is how we are rendering for Keras 3 example( Banner after description): https://keras.io/examples/vision/image_classification_from_scratch/
For Keras 2 example:
https://keras.io/examples/vision/retinanet/
With the changes in this PR, for this banner, the only extra thing is getting added is a warning message(after Keras 2 banner) for Keras 2 examples like below image
So, if I'm understanding it correctly, you are suggesting to replace Keras 2 warning message from "ⓘ This example uses Keras 2" to
(
"Warning: This example uses Keras 2 and may not be compatible with the latest version of Keras. "
"Please check out all of our <a href="https://keras.io/examples/">Keras 3 examples here</a>."
)
If we are replacing this above message in banner, it would be too big, we may have to shorten the message.
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 visual explanation - yes that sounds good, I think it makes sense to move the 2nd Keras 2 message up into the yellow text box so it's more prominent to users. Would the text be able to fit if we added line breaks (<br/>) in the right places?
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.
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, thanks!
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.
Pushed the change, please review it.
|
Thanks for your patience! Lgtm |



This PR updates the documentation generation system to filter Keras 2 examples, ensuring that only Keras 3 content is displayed on keras.io. By adding a Keras_2 tag, legacy guides remain in the repository for future migration reference but will no longer be rendered on the website.