[OpenVINO] Implement cond operator and enable tests for Hinge losses#22409
[OpenVINO] Implement cond operator and enable tests for Hinge losses#22409goyaladitya05 wants to merge 3 commits intokeras-team:masterfrom
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 capabilities by introducing support for conditional operations and expanding test coverage for critical loss functions. The core change involves implementing the 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
|
![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 implements the cond operator for the OpenVINO backend and enables several previously excluded tests. While the implementation enables new functionality, it has a critical issue: the cond operator eagerly evaluates both true_fn and false_fn branches, which is inconsistent with other Keras backends and can lead to incorrect behavior or performance degradation. This should be addressed by using a proper control flow operator like OpenVINO's If operator. Additionally, there's a medium-severity issue regarding one-sided type promotion that could be improved for robustness.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22409 +/- ##
=======================================
Coverage 83.04% 83.04%
=======================================
Files 596 596
Lines 66693 66708 +15
Branches 10383 10387 +4
=======================================
+ Hits 55382 55395 +13
- Misses 8675 8676 +1
- Partials 2636 2637 +1
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:
|
| CosineDecayRestartsTest::test_alpha | ||
| CosineDecayRestartsTest::test_decay | ||
| CosineDecayRestartsTest::test_float64 | ||
| CosineDecayRestartsTest::test_mmul | ||
| CosineDecayRestartsTest::test_tmul | ||
| CosineDecayTest::test_warmup | ||
| CosineDecayTest::test_warmup_decay |
There was a problem hiding this comment.
These have no use case for OpenVINO backend (it being inference-only), these will never be used. But after the implementation of cond, the tests are passing.
After this change, the tests will run in the CI (since they do not have @pytest.mark.requires_trainable_backend decorator) and pass, since they are mathematical tests.
Should I keep them excluded or enable them ?
Or maybe we could have @pytest.mark.requires_trainable_backend decorators for LR schedule releted tests, since they will be used in training only.
There was a problem hiding this comment.
If they pass, let's run them.
The @pytest.mark.requires_trainable_backend was really a way to say "requires auto differentiation (gradients)" support. I think it's fine to still run tests that are training related but don't use gradients like LR schedules.
| true_val = true_fn() | ||
| false_val = false_fn() |
There was a problem hiding this comment.
Hmm... so the whole point of cond is that you should never have to evaluate the value of both true_fn and false_fn.
But I suppose this is a limitation of OpenVino, right?
| if true_val is None: | ||
| return None |
| CosineDecayRestartsTest::test_alpha | ||
| CosineDecayRestartsTest::test_decay | ||
| CosineDecayRestartsTest::test_float64 | ||
| CosineDecayRestartsTest::test_mmul | ||
| CosineDecayRestartsTest::test_tmul | ||
| CosineDecayTest::test_warmup | ||
| CosineDecayTest::test_warmup_decay |
There was a problem hiding this comment.
If they pass, let's run them.
The @pytest.mark.requires_trainable_backend was really a way to say "requires auto differentiation (gradients)" support. I think it's fine to still run tests that are training related but don't use gradients like LR schedules.
4 participants
This PR implements
condoperator for the OpenVINO backend usingov_opset.select, which evaluates both branches as graph nodes and selects the result based on pred.Enabled 9 previously excluded tests for cond, Hinge, and SquaredHinge.
Closes: openvinotoolkit/openvino/issues/34672