-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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
base: master
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this 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.
/gemini review |
There was a problem hiding this 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.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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] |
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