Skip to content
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

[NVIDIA] Update setup.py script #751

Conversation

redradist
Copy link
Contributor

No description provided.

@redradist redradist requested a review from a team as a code owner October 24, 2023 09:48
@github-actions github-actions bot added category: NVIDIA plugin OpenVINO NVIDIA plugin dependencies Pull requests that update a dependency file labels Oct 24, 2023
@redradist redradist changed the title Update setup.py script [NVIDIA] Update setup.py script Oct 24, 2023
@redradist redradist force-pushed the refactoring/update-setup-py-script branch from abcf9d5 to 1f3b4dc Compare October 24, 2023 09:50
@redradist redradist force-pushed the refactoring/update-setup-py-script branch from 1f3b4dc to 6b7bd73 Compare October 24, 2023 09:57
f'-DWHEEL_VERSION={WHEEL_VERSION}',
'-DENABLE_WHEEL=ON']
f'-DPYTHON_INCLUDE_DIR={python_include_dir}',
'-DNGRAPH_PYTHON_BUILD_ENABLE=ON',
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilya-lavrenov

PYTHON_EXECUTABLE and similar are replaced with Python3_EXECUTABLE

Why Python3_EXECUTABLE and not PYTHON3_EXECUTABLE ??
I suggest in future to change from Python3_EXECUTABLE -> PYTHON3_EXECUTABLE for consistency reason

Why is cmake generator explicitly specified? It can be autodetected

It is recommended to use which for detecting proper command path, see https://docs.python.org/3/library/subprocess.html#popen-constructor

Copy link
Contributor Author

@redradist redradist Oct 24, 2023

Choose a reason for hiding this comment

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

@ilya-lavrenov What is current alternative to plugins.xml ? How external plugin could register currently ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest in future to change from Python3_EXECUTABLE -> PYTHON3_EXECUTABLE for consistency reason

You can make this proposal to cmake 😄 https://cmake.org/cmake/help/latest/module/FindPython3.html

It is recommended to use which for detecting proper command path, see

I meant -G option lists makefiles in your case. What if make command is missed, while ninja-build is installed?

How external plugin could register currently ?

As I see you compile NVIDIA plugin with main OpenVINO, which means NVIDIA plugin is not external and plugins.xml is not required.
When plugin is built on top of prebuilt OpenVINO, them plugins.xml is auto-generated and plugin is responsible for its installation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilya-lavrenov

I suggest in future to change from Python3_EXECUTABLE -> PYTHON3_EXECUTABLE for consistency reason

You can make this proposal to cmake 😄 https://cmake.org/cmake/help/latest/module/FindPython3.html

Yeah, I thought it is internal OpenVINO option :) Just confused myself )

It is recommended to use which for detecting proper command path, see

I meant -G option lists makefiles in your case. What if make command is missed, while ninja-build is installed?

It is related to commands passed to subprocess, which Python tries to find

How external plugin could register currently ?

As I see you compile NVIDIA plugin with main OpenVINO, which means NVIDIA plugin is not external and plugins.xml is not required. When plugin is built on top of prebuilt OpenVINO, them plugins.xml is auto-generated and plugin is responsible for its installation.

Maybe I can compile it with -DENABLE_PLUGINS_XML=ON ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is related to commands passed to subprocess, which Python tries to find

I don't understand how it's connected with subprocesses and Python, maybe I'm missing something. I suppose just need to remove -G to allow cmake to find generator on the system automatically.

Maybe I can compile it with -DENABLE_PLUGINS_XML=ON ?

If you are building whole OpenVINO in this setup.py, then yes - it can be a solution (just to fallback to old behavior)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: NVIDIA plugin OpenVINO NVIDIA plugin dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants