Skip to content

Support numpy.prod operation #21188

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Prabha14039
Copy link

@Prabha14039 Prabha14039 commented Apr 19, 2025

Details

fixes openvinotoolkit/openvino#30212

  • Implements reduce_prod using OpenVINO opset
  • Handles dtype, axis, and keepdims parameters
  • Adds necessary conversion and reduction logic

cc, @rkazants please review 🙏

Copy link

google-cla bot commented Apr 19, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Prabha14039 Prabha14039 marked this pull request as ready for review April 19, 2025 18:46
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2025

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 74.19%. Comparing base (dd9132f) to head (a198e04).

Files with missing lines Patch % Lines
keras/src/backend/openvino/numpy.py 0.00% 19 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (dd9132f) and HEAD (a198e04). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (dd9132f) HEAD (a198e04)
keras 5 3
keras-openvino 1 0
keras-tensorflow 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #21188      +/-   ##
==========================================
- Coverage   82.59%   74.19%   -8.40%     
==========================================
  Files         564      564              
  Lines       54407    54426      +19     
  Branches     8449     8453       +4     
==========================================
- Hits        44936    40383    -4553     
- Misses       7396    11971    +4575     
+ Partials     2075     2072       -3     
Flag Coverage Δ
keras 74.05% <0.00%> (-8.36%) ⬇️
keras-jax 63.78% <0.00%> (-0.03%) ⬇️
keras-numpy 58.90% <0.00%> (-0.03%) ⬇️
keras-openvino ?
keras-tensorflow ?
keras-torch 63.87% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pytest.ini Outdated
@@ -0,0 +1,3 @@
[pytest]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this file. It is only for developer purpose

requirements.txt Outdated
Comment on lines 16 to 18
jax[cpu]==0.5.0

#removed jax as is not nedded in our case
#jax[cpu]==0.5.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this change

@rkazants
Copy link
Contributor

Please fix CI failures as well

@Prabha14039
Copy link
Author

I tried running the pytest locally but didn't get any error could guide how to run these test

@11happy
Copy link
Contributor

11happy commented Apr 20, 2025

I tried running the pytest locally but didn't get any error could guide how to run these test

Try this & then run pytests
export KERAS_BACKEND=openvino

@Prabha14039
Copy link
Author

I tried running the pytest locally but didn't get any error could guide how to run these test

Try this & then run pytests export KERAS_BACKEND=openvino

thank you
it was actually the export issue cause of which the failed test were not showing

@Prabha14039
Copy link
Author

cc, @rkazants please review

@11happy
Copy link
Contributor

11happy commented Apr 20, 2025

there are 2 more CI issues, first you need to sign the CLA aggrement https://cla.developers.google.com/
after run pre-commit to remove the formatting issues, you can follow this https://github.com/keras-team/keras/blob/master/CONTRIBUTING.md

@Prabha14039
Copy link
Author

Apologies for not following the contribution guidelines initially.
I've now applied the required changes as per the project's standards.

cc @rkazants — kindly review when you get a chance. Thank you!

Comment on lines 1152 to 1163
# Promote dtype if not explicitly specified
if dtype is None:
if x_type == Type.boolean:
promoted_dtype = Type.i32
elif x_type in (Type.i8, Type.i16):
promoted_dtype = Type.i32
elif x_type in (Type.u8, Type.u16):
promoted_dtype = Type.u32
else:
promoted_dtype = x_type
else:
promoted_dtype = string_to_ov_type(dtype)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need any type promotion here. Please remove

Copy link
Author

@Prabha14039 Prabha14039 Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the dtype promotion as you suggested (like uint8 → uint32, etc.), but now a few CI tests are failing because they still expect the promoted dtype (e.g., jnp.prod(uint8) → uint32, but OpenVINO now stays uint8).Could you please let me know that should i remove it or not or waht other ways should i try it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have provided the test case that is failing
image

requirements.txt Outdated
@@ -13,6 +13,7 @@ torch-xla==2.6.0;sys_platform != 'darwin'
# Jax.
# Pinned to 0.5.0 on CPU. JAX 0.5.1 requires Tensorflow 2.19 for saved_model_test.
# Note that we test against the latest JAX on GPU.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Good First Issue][Keras 3 OpenVINO Backend]: Support numpy.prod operation
5 participants