Skip to content

Openvino backend improvements #21381

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 2 commits into
base: master
Choose a base branch
from

Conversation

Mezalfa
Copy link

@Mezalfa Mezalfa commented Jun 14, 2025

This pull request implements keras.numpy.prod for the OpenVINO backend.

Features:
Handles reduction along specified axes, including single integers, tuples, and None.
Implements promotion for dtype when dtype is passed as None ,converting to a suitable OpenVINO type.
If dtype is specified, the output is converted to the specified dtype.
Properly manages the keepdims argument to retain or drop the reduced dimensions in the output shape.

In addition to numpy.prod, the openvino.runtime import style has been changed to the newer openvino import style in the openvino backend folder.

@rkazants

Copy link

google-cla bot commented Jun 14, 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.

Copy link
Contributor

@rkazants rkazants left a comment

Choose a reason for hiding this comment

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

do NOT merge!!!
huge amount of changes, not possible to review at all.

@divyashreepathihalli
Copy link
Collaborator

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 successfully implements numpy.prod for the OpenVINO backend and updates the import style for the openvino package, which is a great improvement. The implementation of prod looks solid.

However, a critical issue was found: the entire venv_keras_ov virtual environment directory has been included in the commit. This should be removed from the pull request and added to the .gitignore file to prevent bloating the repository and including user-specific files. I've also included one minor suggestion to improve code clarity in the prod implementation.

Comment on lines +1 to +66
# This file must be used with "source bin/activate" *from bash*
# you cannot run it directly

deactivate () {
# reset old environment variables
if [ -n "${_OLD_VIRTUAL_PATH:-}" ] ; then
PATH="${_OLD_VIRTUAL_PATH:-}"
export PATH
unset _OLD_VIRTUAL_PATH
fi
if [ -n "${_OLD_VIRTUAL_PYTHONHOME:-}" ] ; then
PYTHONHOME="${_OLD_VIRTUAL_PYTHONHOME:-}"
export PYTHONHOME
unset _OLD_VIRTUAL_PYTHONHOME
fi

# This should detect bash and zsh, which have a hash command that must
# be called to get it to forget past commands. Without forgetting
# past commands the $PATH changes we made may not be respected
if [ -n "${BASH:-}" -o -n "${ZSH_VERSION:-}" ] ; then
hash -r 2> /dev/null
fi

if [ -n "${_OLD_VIRTUAL_PS1:-}" ] ; then
PS1="${_OLD_VIRTUAL_PS1:-}"
export PS1
unset _OLD_VIRTUAL_PS1
fi

unset VIRTUAL_ENV
if [ ! "${1:-}" = "nondestructive" ] ; then
# Self destruct!
unset -f deactivate
fi
}

# unset irrelevant variables
deactivate nondestructive

VIRTUAL_ENV="/Users/mesmergebrealfa/keras/venv_keras_ov"
export VIRTUAL_ENV

_OLD_VIRTUAL_PATH="$PATH"
PATH="$VIRTUAL_ENV/bin:$PATH"
export PATH

# unset PYTHONHOME if set
# this will fail if PYTHONHOME is set to the empty string (which is bad anyway)
# could use `if (set -u; : $PYTHONHOME) ;` in bash
if [ -n "${PYTHONHOME:-}" ] ; then
_OLD_VIRTUAL_PYTHONHOME="${PYTHONHOME:-}"
unset PYTHONHOME
fi

if [ -z "${VIRTUAL_ENV_DISABLE_PROMPT:-}" ] ; then
_OLD_VIRTUAL_PS1="${PS1:-}"
PS1="(venv_keras_ov) ${PS1:-}"
export PS1
fi

# This should detect bash and zsh, which have a hash command that must
# be called to get it to forget past commands. Without forgetting
# past commands the $PATH changes we made may not be respected
if [ -n "${BASH:-}" -o -n "${ZSH_VERSION:-}" ] ; then
hash -r 2> /dev/null
fi

Choose a reason for hiding this comment

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

critical

It appears that the entire venv_keras_ov virtual environment directory has been committed to the repository. This is generally considered a bad practice and should be avoided for several reasons:

  • Repository Bloat: It adds a large number of files, including binaries, to the repository, significantly increasing its size.
  • User-Specific Paths: The activation scripts contain hardcoded absolute paths specific to your machine (e.g., /Users/mesmergebrealfa/keras/venv_keras_ov), which will not work for other developers.
  • Security Risk: Virtual environments can potentially contain sensitive information or dependencies that should not be shared.
  • Non-Portability: The contents are specific to the operating system and Python version used to create it.

It is strongly recommended to remove this directory from the pull request and add venv_keras_ov/ (or a more general pattern like venv*/) to the project's .gitignore file to prevent it from being committed in the future.

Comment on lines +1301 to +1306
target_type = None
if dtype is None:
target_type = promotion_map.get(x_type, x_type)
else:
dtype_string = standardize_dtype(dtype)
target_type = OPENVINO_DTYPES[dtype_string]

Choose a reason for hiding this comment

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

medium

The variable target_type is initialized to None on line 1301, but it is immediately reassigned in the following if/else block, making the initial assignment redundant. Removing this line will make the code slightly cleaner.

Suggested change
target_type = None
if dtype is None:
target_type = promotion_map.get(x_type, x_type)
else:
dtype_string = standardize_dtype(dtype)
target_type = OPENVINO_DTYPES[dtype_string]
if dtype is None:
target_type = promotion_map.get(x_type, x_type)
else:
dtype_string = standardize_dtype(dtype)
target_type = OPENVINO_DTYPES[dtype_string]

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.

4 participants