-
Couldn't load subscription status.
- Fork 306
Fix tensorflow-text import to not break core tensorflow functionality #2448
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 tensorflow-text import to not break core tensorflow functionality #2448
Conversation
Summary of ChangesHello @nikolasavic3, 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 refactors the import mechanism for 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 correctly separates the import handling for tensorflow and tensorflow-text to prevent issues when one is available but not the other. This is a good improvement for robustness. However, there is a critical syntax error in the new except block that needs to be fixed. I've provided a suggestion to correct it.
e0b9991 to
982f4b7
Compare
982f4b7 to
d5c3290
Compare
|
/gemini review |
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 correctly addresses an issue where a failure to import tensorflow-text would incorrectly cause tensorflow to be considered unavailable. By separating the import try-except blocks, the library can now function correctly when tensorflow is installed but tensorflow-text is not. My review includes a suggestion to further clean up the new import logic for better clarity and maintainability.
| try: | ||
| import tensorflow_text as tf_text | ||
| except ImportError: | ||
| tf_text = None |
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.
Can you remove line 17? (tf_text = None in the other except block)
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.
Done. Should we nest the tensorflow_text import inside the tensorflow import block to avoid attempting to import tf_text when tf isn't available?
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.
Not a big deal, it tf fails, tensorflow_text should fail too, and regardless nothing using either of those will work.
| try: | ||
| import tensorflow_text as tf_text | ||
| except ImportError: | ||
| tf_text = None |
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.
Not a big deal, it tf fails, tensorflow_text should fail too, and regardless nothing using either of those will work.
|
Thank you for the fix! |
Description
Separates the try-except blocks for
tensorflowandtensorflow-textimports to allow the library to function when tensorflow-text is unavailable (e.g., Python 3.13 where tensorflow-text support is not yet available).Problem
Currently, if
tensorflow-textfails to import, bothtfandtf_textare set toNone, even iftensorflowimports successfully. This causesAttributeError: 'NoneType' object has no attribute 'RaggedTensor'errors in downstream libraries.Related Issues