[OpenVINO] Fix array(), maximum(), minimum(), reshape() and split() ops in the numpy suite#22343
Conversation
Summary of ChangesHello, 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 significantly enhances the OpenVINO backend's NumPy suite by refining the implementation of several core operations, including 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. Changelog
Activity
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.
Code Review
This pull request introduces several important fixes and refactorings for the OpenVINO numpy backend, improving consistency and correctness for array, maximum, minimum, reshape, and split operations. A key improvement is the _promote_binary_types helper for consistent type promotion. The addition of __iter__ to OpenVINOKerasTensor is a critical fix for an infinite loop issue. I've identified a bug in the reshape implementation where tensor-shaped newshape arguments are not handled, and a limitation in split where it only supports constant tensor inputs for indices_or_sections.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22343 +/- ##
==========================================
- Coverage 83.01% 76.41% -6.60%
==========================================
Files 596 596
Lines 66595 66591 -4
Branches 10369 10372 +3
==========================================
- Hits 55281 50886 -4395
- Misses 8677 13273 +4596
+ Partials 2637 2432 -205
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:
|
92d9735 to
f5a4432
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes and improvements for the OpenVINO backend. The addition of __iter__ to OpenVINOKerasTensor is a critical fix to prevent potential infinite loops. The refactoring of binary operations into a _promote_binary_types helper cleans up the code and correctly handles type promotion for maximum and minimum with Python scalars. Other fixes for array, reshape, and split enhance consistency and functionality. Overall, these are great changes. I have one suggestion to make the reshape implementation even more robust.
|
I've rebased the PR, and all tests are passing. Please review. Thanks! |
…plit() numpy ops - array(): use convert_to_tensor() instead of np.array() so the result is always an OpenVINOKerasTensor rather than a raw NumPy array - maximum() / minimum(): apply the same t1/t2 dtype-extraction pattern used by add() / subtract() / multiply() so that Python int/float literals are promoted according to JAX weak-type semantics - reshape(): wrap a bare integer newshape in a list before passing it to ov_opset.constant(), which requires a 1-D shape tensor - split(): extract constant data from an OpenVINOKerasTensor passed as indices_or_sections (side-effect of the array() fix); also fixes hsplit() and vsplit() which delegate to split()
…ate binary op type promotion
f5b72ed to
4ea4368
Compare
Closing since the other one was submitted. Please re-open if this is not the case. |
|
@hertschuh Could you reopen this please? I'm not getting an option to reopen it myself. |
|
Done. Please rebase. |
Removed several ReversibleEmbeddingTest cases from the excluded tests list.
This PR focuses on improving the implementations for
array(), maximum(), minimum(), reshape() and split()for the OpenVINO Backend in the numpy suite.array()- replacednp.array()withconvert_to_tensor()so the return value is always anOpenVINOKerasTensorinstead of a raw NumPy array, making it consistent with other ops in the backend.maximum()/minimum()- added thet1/t2dtype-extraction step (same pattern asadd(),subtract(),multiply()) so Pythonint/floatliterals are promoted using JAX weak-type semantics instead of being unconditionally cast toint32/float32reshape()- added a guard for integernewshape(e.g.Reshape(-1)), wrapping it in a list before passing toov_opset.constant()which requires a 1-D shape tensorsplit()- handleOpenVINOKerasTensorasindices_or_sectionsby extracting the underlying constant data viaget_node().get_data(); this was a side-effect of thearray()fix and also covershsplit()/vsplit()which delegate tosplit()Duplicate of #22319
The CI was getting stuck, so I looked deeper and found the main issue.
OpenVINO tensors rely on sequence style iteration via
__getitem__, but__getitem__never raisesIndexError, so loops likezip(mask, mask2)become unbounded and CI appears stuck. I’m adding a bounded iter on OpenVINOKerasTensor to make iteration finite and deterministic.Closes: openvinotoolkit/openvino/issues/34409