Arm backend: Pin pybind11 version when installing tosa_serialization#18036
Arm backend: Pin pybind11 version when installing tosa_serialization#18036tom-arm wants to merge 3 commits intopytorch:mainfrom
Conversation
* To account for potential edge cases of pybind11 drifting * Will be removed when tosa-tools is installed from pypi Signed-off-by: Tom Allsop <tom.allsop@arm.com> Change-Id: I197d3c141bd8918a8debaba55adcdc9fb296ba17
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18036
Note: Links to docs will display an error until the docs builds have been completed. ❌ 5 New Failures, 1 Cancelled Job, 1 Pending, 1 Unrelated FailureAs of commit 48d0a73 with merge base e458023 ( NEW FAILURES - The following jobs have failed:
CANCELLED JOB - The following job was cancelled. Please retry:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
There was a problem hiding this comment.
Pull request overview
Pins pybind11 to a specific version to make local/CI installation of tosa_serialization more deterministic and avoid issues from dependency drift until tosa-tools can be installed from PyPI.
Changes:
- Add
pybind11==2.10.4to the ARM TOSA requirements file. - Explicitly install
pybind11==2.10.4in the Linux CMake unit test CI script before installingtosa_serialization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backends/arm/requirements-arm-tosa.txt | Pins pybind11 for ARM/TOSA dependency installs. |
| .ci/scripts/unittest-linux-cmake.sh | Installs pinned pybind11 in CI prior to building/installing tosa_serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -7,3 +7,4 @@ ml_dtypes == 0.5.1 | |||
| flatbuffers == 24.3.25 | |||
| tosa-adapter-model-explorer == 0.1.0 | |||
| ai-edge-model-explorer >= 0.1.16 | |||
There was a problem hiding this comment.
The pybind11 version is pinned here, but the same version is also hard-coded in .ci/scripts/unittest-linux-cmake.sh. This duplication is easy to let drift and can create CI vs local setup mismatches. Consider centralizing the pin (e.g., a shared constraints file or having CI install from this requirements file) or add an explicit note that the CI script must be kept in sync.
| ai-edge-model-explorer >= 0.1.16 | |
| ai-edge-model-explorer >= 0.1.16 | |
| # NOTE: pybind11 version must be kept in sync with .ci/scripts/unittest-linux-cmake.sh |
| TOSA_SERIALIZATION_DIR="${TOSA_TOOLS_DIR}/serialization" | ||
| fi | ||
|
|
||
| python -m pip install pybind11==2.10.4 |
There was a problem hiding this comment.
This pybind11 pin duplicates the version in backends/arm/requirements-arm-tosa.txt. To reduce the chance of the two pins diverging, consider deriving the version from a single source (e.g., install -r backends/arm/requirements-arm-tosa.txt/a shared constraints file, or define a single variable that both paths use).
| python -m pip install pybind11==2.10.4 | |
| python -m pip install -r backends/arm/requirements-arm-tosa.txt |
There was a problem hiding this comment.
This will install unnecessary dependencies, so I will not take this suggestion
zingo
left a comment
There was a problem hiding this comment.
This reverts a previous fix we had the we remove last week but it is still needed so we are putting it back again. Sorry for the pingponging with this.
Change-Id: I8385d18b84eadb1cc59218edfe2cce72765d12a9
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Change-Id: I197d3c141bd8918a8debaba55adcdc9fb296ba17
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218 @mansnils @Sebastian-Larsson @robell