Add _maybe_convert_to_int utility to handle symbolic tensor dimensions safely #21848
Conversation
Summary of ChangesHello @MalyalaKarthik66, 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 introduces a new helper utility, 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility function, _maybe_convert_to_int, to safely convert values, including symbolic tensors, to integers. This is a valuable addition for improving backend compatibility. The accompanying test case effectively demonstrates the utility's functionality within a Dense layer. My review focuses on enhancing the robustness of the new utility function by improving its error handling.
| def _maybe_convert_to_int(x: Any) -> Any: | ||
| if isinstance(x, int): | ||
| return x | ||
| if isinstance(x, (tuple, list)): | ||
| try: | ||
| return tuple(int(v) for v in x) | ||
| except Exception: | ||
| return x | ||
|
|
||
| try: | ||
| np_val = ops.convert_to_numpy(x) | ||
| except Exception: | ||
| return x | ||
|
|
||
| if np.isscalar(np_val): | ||
| try: | ||
| return int(np_val) | ||
| except Exception: | ||
| return x | ||
|
|
||
| arr = np.asarray(np_val).ravel() | ||
| if arr.size == 0: | ||
| return x | ||
| if arr.size == 1: | ||
| return int(arr[0]) | ||
| try: | ||
| return tuple(int(v) for v in arr.tolist()) | ||
| except Exception: | ||
| return x |
There was a problem hiding this comment.
The function can be made more robust and slightly cleaner. Using except Exception for int() conversions is too broad and can mask unexpected errors. It's better to catch specific exceptions like ValueError and TypeError. Additionally, the conversion int(arr[0]) on line 32 is not wrapped in a try...except block and could raise an unhandled exception if the element is not convertible to an integer.
I've suggested a refactoring that addresses these points by using more specific exceptions and ensuring all integer conversions are safely handled. The broad except Exception for ops.convert_to_numpy is kept, as it's intended to handle various failures from different backends, especially for symbolic tensors.
def _maybe_convert_to_int(x: Any) -> Any:
if isinstance(x, int):
return x
if isinstance(x, (tuple, list)):
try:
return tuple(int(v) for v in x)
except (ValueError, TypeError):
return x
try:
np_val = ops.convert_to_numpy(x)
except Exception:
return x
if np.isscalar(np_val):
try:
return int(np_val)
except (ValueError, TypeError):
return x
arr = np.asarray(np_val).ravel()
if arr.size == 0:
return x
try:
if arr.size == 1:
return int(arr[0])
return tuple(int(v) for v in arr.tolist())
except (ValueError, TypeError):
return x
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21848 +/- ##
==========================================
- Coverage 82.66% 82.64% -0.02%
==========================================
Files 577 579 +2
Lines 59477 59564 +87
Branches 9329 9335 +6
==========================================
+ Hits 49167 49228 +61
- Misses 7907 7927 +20
- Partials 2403 2409 +6
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:
|
3 participants
Fix: #21655
This PR adds a helper utility _maybe_convert_to_int to safely handle cases where shape dimensions or layer units are symbolic tensors. It ensures that numeric values are properly converted to integers when possible, improving backend compatibility.