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

[PyOV] Migrate wheel building interface to python -m build for main OV wheel #27190

Merged
merged 5 commits into from
Oct 23, 2024

Conversation

p-wysocki
Copy link
Contributor

Details:

  • Direct setup.py calls are deprecated and using build package instead is recommended (source)
  • PR simplifies wheel building by using python -m build for all pip versions
  • A new build dependency has been added since build is not supplied with Python
  • setuptools (our current build backend) is not integrated well with config_settings, which leads to an unfriendly looking workaround for passing build settings (more details in Setuptools does not pass config_settings through backend pypa/setuptools#2491)
  • Migrations for other wheels will be delivered in separate PRs as they have a different scope (for example openvino_dev also requires migration from using pkg_resources package)

Tickets:

@p-wysocki p-wysocki requested review from a team as code owners October 22, 2024 15:23
@p-wysocki p-wysocki requested review from artanokhov and removed request for a team October 22, 2024 15:23
@github-actions github-actions bot added category: build OpenVINO cmake script / infra category: Python API OpenVINO Python bindings category: dependency_changes Pull requests that update a dependency file category: packaging OpenVINO packaging / distribution labels Oct 22, 2024
@mlukasze mlukasze enabled auto-merge October 23, 2024 09:14
@mlukasze mlukasze added this pull request to the merge queue Oct 23, 2024
Merged via the queue into openvinotoolkit:master with commit 1b0a4cf Oct 23, 2024
159 of 161 checks passed
@p-wysocki p-wysocki deleted the setuppy_migration branch October 24, 2024 08:37
endif()
# for --config-setting explanation see https://github.com/pypa/setuptools/issues/2491
set(wheel_build_command
${Python3_EXECUTABLE} -m build "${CMAKE_CURRENT_SOURCE_DIR}"
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, should we use pip wheel instead ? to avoid dependency on build

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

@p-wysocki p-wysocki Nov 6, 2024

Choose a reason for hiding this comment

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

BTW, should we use pip wheel instead ? to avoid dependency on build

In the original code there was an if depending on pip version, and for older pips it would call setup.py directly. I tried unifying the functionality to call pip wheel for all pip versions, but it seems there's some incompatibility which results in very unfriendly errors which would require (in my opinion) a lot of changes, only once I figure out what needs to be changed.

Changing to using build (which is a pypa recommended replacement) works for all pip versions and removes the branching based on pip version.

We could work on making pip wheel work with old pip versions, but I believe it will be time consuming and difficult, or stay with pip wheel but drop support for older pips.

BTW, are you going to rework this call as well ?
https://github.com/openvinotoolkit/openvino/blob/master/src/bindings/python/CMakeLists.txt#L328

The changes regarding this call are in #27227.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can enable pip wheel for newer pip versions, while old ones (via build) will not be used after some time and we can just force to use newer pip only (once Ubuntu 20.04 is dropped, for example)

CuriousPanCake pushed a commit to CuriousPanCake/openvino that referenced this pull request Nov 6, 2024
… OV wheel (openvinotoolkit#27190)

### Details:
- Direct `setup.py` calls are deprecated and using `build` package
instead is recommended
([source](https://packaging.python.org/en/latest/discussions/setup-py-deprecated/))
- PR simplifies wheel building by using `python -m build` for all pip
versions
- A new build dependency has been added since `build` is not supplied
with Python
- `setuptools` (our current build backend) is not integrated well with
`config_settings`, which leads to an unfriendly looking workaround for
passing build settings (more details in
pypa/setuptools#2491)
- Migrations for other wheels will be delivered in separate PRs as they
have a different scope (for example `openvino_dev` also requires
migration from using `pkg_resources` package)

### Tickets:
 - CVS-155190
github-merge-queue bot pushed a commit that referenced this pull request Nov 6, 2024
### Details:
- An alternative to
#27426
- The functionality branching based on pip version has recently been
removed in #27190
- This fix has originated from discussion at
https://github.com/openvinotoolkit/openvino/pull/27190/files#r1830707810
 - Fix for issues with NPU building on Windows machines
- A disadvantage is that we need to have `build` as a dependency no
matter what, there's no environment marker for `pip` version which would
allow us to install it conditionally

### Tickets:
 - EISW-146038

Signed-off-by: p-wysocki <[email protected]>
NishantPrabhuFujitsu pushed a commit to NishantPrabhuFujitsu/openvino that referenced this pull request Nov 26, 2024
…lkit#27429)

### Details:
- An alternative to
openvinotoolkit#27426
- The functionality branching based on pip version has recently been
removed in openvinotoolkit#27190
- This fix has originated from discussion at
https://github.com/openvinotoolkit/openvino/pull/27190/files#r1830707810
 - Fix for issues with NPU building on Windows machines
- A disadvantage is that we need to have `build` as a dependency no
matter what, there's no environment marker for `pip` version which would
allow us to install it conditionally

### Tickets:
 - EISW-146038

Signed-off-by: p-wysocki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: build OpenVINO cmake script / infra category: dependency_changes Pull requests that update a dependency file category: packaging OpenVINO packaging / distribution category: Python API OpenVINO Python bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants