-
Notifications
You must be signed in to change notification settings - Fork 168
[NVIDIA] Prepare setup.py for PyPi #1001
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] Prepare setup.py for PyPi #1001
Conversation
b7942b2 to
c95a838
Compare
|
@ilya-lavrenov @p-durandin Now cherry on cake - python package |
|
build_jenkins |
p-wysocki
left a comment
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.
While the README.md file makes sense, there are a lot of typos and its style could be aligned to the style of the rest of the file. Could you please correct it or at least run it through an LLM to improve formatting and typos?
| ### Building the package | ||
| To build it, use simply the following command: | ||
| ```bash | ||
| python3 ./wheel/setup.py bdist_wheel |
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.
Calling bdist_wheel manually has been deprecated for a while, going as far as an official Python blog saying that they mustn't be run anymore. The recommended replacement is python -m build which requires build dependency. Could you please check if running it is compatible and works well? We shouldn't introduce functionality based on outdated, possibly broken command.
Same goes for setup.py install below.
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 functionality could be done iteratively.
In my current python environment python -m build failed due to missed dependencies (probably because I have custom build for python)
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.
I'm not a fan of introducing a functionality which we know may be broken/is not only not recommended, but soft-forbidden by Python docs. These things tend to stick around as a "TODO" and later cause issues at the most inconvenient time.
I haven't maintained NVIDIA plugin before so IMO my opinion can be overridden if you wish to do so (cc @p-durandin). If we decide to merge it as-is at the very least let's create a ticket to update the build method.
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.
@p-wysocki thanks, lets merge with creating github issue
c95a838 to
f70c98c
Compare
…as provided and openvino downloaded from the internet
… plugin; update README.,d by adding the section related to how build python package for NVIDIA plugin
f70c98c to
42c4f44
Compare
@p-wysocki Looks like fixed all typos |
p-wysocki
left a comment
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.
Overall LGTM if it wasn't for the outdated wheel build/install method I mentioned in one of my comments. I'd prefer it to be done properly to avoid problems in the future.
Should we update the CI somehow to test this? I'm not familiar with openvino_contrib pipelines.
@p-durandin, is there a designated maintainer for this repo?
|
|
||
| __version__ = "2024.1.0" | ||
|
|
||
| __version__ = "2025.3.0" |
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.
Perhaps we should be setting this dynamically based on openvino version, or at least set it to an upcoming 2025.4.0 if we plan to release it? I'm not sure how it should be done, @artanokhov do you know?
One way or another, if we'd like to set this dynamically, this is a topic for a different PR.
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.
Perhaps we should be setting this dynamically based on
openvinoversion, or at least set it to an upcoming2025.4.0if we plan to release it?
@p-wysocki Have not idea, I am not the official maintainer of NVIDIA plugin, I am the former developer of it and I know pretty good its architecture.
In future I expect Intel will publish packages for openvino-nvidia, but for this proper CI should be configured
| ### Building the package | ||
| To build it, use simply the following command: | ||
| ```bash | ||
| python3 ./wheel/setup.py bdist_wheel |
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.
I'm not a fan of introducing a functionality which we know may be broken/is not only not recommended, but soft-forbidden by Python docs. These things tend to stick around as a "TODO" and later cause issues at the most inconvenient time.
I haven't maintained NVIDIA plugin before so IMO my opinion can be overridden if you wish to do so (cc @p-durandin). If we decide to merge it as-is at the very least let's create a ticket to update the build method.
|
@p-wysocki I've played with it little bit, it requires some changes here and there to implement it using |
|
build_jenkins |
|
Overall LGTM if it wasn't for the |
|
To be followed in #1011 |
Description
Update
setup.pyto be usable for PyPi.Create initial package for
openvino-nvidiaplugin in PyPi, see https://pypi.org/project/openvino-nvidia