-
Notifications
You must be signed in to change notification settings - Fork 19.6k
Document that set_backend requires re-importing keras.
#21764
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
Both in the example and the documentation.
Summary of ChangesHello @hertschuh, 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 clarity of 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 improves the documentation for keras.config.set_backend by clarifying that the keras module must be re-imported after changing the backend. The changes in both the code example and the warning text are clear and helpful for users. I've suggested a small improvement to the code example to make it even more illustrative of the function's effect.
| import keras | ||
| keras.config.set_backend("jax") | ||
| del keras | ||
| import keras |
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 example is good, but it could be more illustrative by showing the effect of changing the backend. A slightly more verbose example that prints the backend name before and after the change would make it clearer to users why re-importing keras is necessary and what the function accomplishes. This aligns with the style guide's emphasis on showing how to use the API to solve problems.1
| import keras | |
| keras.config.set_backend("jax") | |
| del keras | |
| import keras | |
| import keras | |
| print(f"Backend before: {keras.backend.backend()}") | |
| keras.config.set_backend("jax") | |
| del keras | |
| import keras | |
| print(f"Backend after: {keras.backend.backend()}") |
Style Guide References
Footnotes
-
The style guide recommends that documentation should show code examples for end-to-end workflows to demonstrate how to use the API, rather than just talking about how it works. A more illustrative example better serves this purpose. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21764 +/- ##
=======================================
Coverage 82.69% 82.69%
=======================================
Files 573 573
Lines 58888 58888
Branches 9218 9218
=======================================
Hits 48696 48696
Misses 7845 7845
Partials 2347 2347
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:
|
| import keras | ||
| ``` | ||
| ⚠️ WARNING ⚠️: Using this function is dangerous and should be done |
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.
Perhaps it would good to print/display some of this warning text when set_backend() is run, otherwise it looks like a silent failure (when a set_backend() is called but the backend doesn't actually change).
If concerned about spamming text, perhaps there can be a flag to suppress the warning (but default to printing something, since this is such a "dangerous" function)
fchollet
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.
In fact, it requires reimporting every single thing imported from Keras! And redefining any function that was using them I believe?
Addressing comments from keras-team#21764
Addressing comments from keras-team#21764
Addressing comments from keras-team#21764
Addressing comments from #21764
Both in the example and the documentation.