PyPi Package Deployment + Remove Banshee Dept #154
Conversation
…y into pr/package-deployment
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces local Banshee builds with prebuilt downloads, removes Rust/cargo from toolchain and containers, adds a Publish GitHub Actions workflow for PyPI/TestPyPI, updates Makefile/CMake and Dockerfiles to use BANSHEE_INSTALL_DIR, relaxes an ONNX dependency, adds releasing docs, and coerces constant tensors before ONNX export. Changes
Sequence Diagram(s)mermaid Dev->>Repo: push tag "v*" or trigger workflow_dispatch Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.github/workflows/_select-env.yml:
- Line 37: Replace the fallback container image value assigned to IMAGE
(currently set to "ghcr.io/victor-jung/deeploy:package-deployment") with the
org-owned registry image "ghcr.io/pulp-platform/deeploy:devel" so CI does not
depend on a personal registry; update the IMAGE assignment accordingly wherever
that fallback is defined.
In `@Deeploy/DeeployTypes.py`:
- Around line 3106-3109: The current loop in DeeployTypes.py unconditionally
casts every gs.Constant's tensor.values to float32 (using tensor.values.astype),
which will corrupt integer/bool/quantized constants; change the logic that
builds or iterates constTensors (from self.graph.tensors().values() and
gs.Constant) to only perform the cast when the constant's dtype is already a
floating type (use numpy's type check, e.g., np.issubdtype(tensor.values.dtype,
np.floating)) and skip casting for integer/bool/quantized tensors so only true
float tensors are converted to np.float32 before export.
In `@docs/releasing.md`:
- Line 17: Clarify that tags are not created automatically on merge to main;
update the sentence about committing/merging to state who must create the
release tag and how (e.g., manually with git tag <version> && git push origin
<tag> or via your release tooling), and note that the release workflow is
triggered by tags matching the pattern "v*" so the tag must be created/pushed to
trigger the workflow; reference the branch name "main" and the tag pattern "v*"
in the new line for clarity.
In `@Makefile`:
- Around line 55-70: The Darwin branch currently hardcodes TARGET :=
aarch64-apple-darwin which mistargets Intel Macs; update the conditional logic
that uses OS and ARCH to check ARCH when OS is Darwin (similar to the Linux
block) and set TARGET to x86_64-apple-darwin for ARCH == x86_64 and
aarch64-apple-darwin for ARCH == aarch64, otherwise emit an unsupported
architecture error; ensure you modify the Makefile's OS/ARCH conditional section
and preserve the existing Linux checks (TARGET, OS, ARCH symbols) while removing
any unfounded special-casing about arm64 on Linux.
- Around line 520-524: The Makefile rule that populates ${BANSHEE_INSTALL_DIR}
currently downloads prebuilt banshee via the curl/tar pipeline using the
$(TARGET) variable without any integrity checks; update this rule to verify the
downloaded artifact before extracting by either fetching a corresponding
checksum or signature and validating it (e.g., download a .sha256 or .asc
alongside the banshee-0.5.0-$(TARGET).tar.gz and run sha256sum -c or gpg
--verify), or if no machine-readable checksum exists, pin and document an
expected SHA256 value and compare it after download; ensure the build fails on
mismatch and reference the same symbols (${BANSHEE_INSTALL_DIR}, TARGET, the
curl download of banshee-0.5.0-$(TARGET).tar.gz) so the change is applied to the
right Makefile target.
🧹 Nitpick comments (3)
pyproject.toml (1)
6-7: Consider adding an upper bound for onnx-graphsurgeon.Relaxing to
>=0.5.8may allow future breaking changes. If you only support a tested major/minor series, add an upper bound and verify compatibility with the latest allowed version.📌 Suggested constraint
-'onnx-graphsurgeon>=0.5.8', +'onnx-graphsurgeon>=0.5.8,<0.6',Also applies to: 25-25
cmake/simulation.cmake (1)
45-52: Validate the banshee executable path.Line 48 sets
BANSHEE_EXECUTABLEbut doesn’t verify it exists. A quick existence check gives a clearer error when the env var is mis-set.✅ Suggested validation
set(BANSHEE_EXECUTABLE "$ENV{BANSHEE_INSTALL_DIR}/banshee") +if(NOT EXISTS "${BANSHEE_EXECUTABLE}") + message(FATAL_ERROR "Banshee executable not found at ${BANSHEE_EXECUTABLE}") +endif()Container/Dockerfile.deeploy (1)
17-17: Use a directory in PATH (or drop this line).
/app/benderlooks like a binary path, not a directory. PATH entries should be directories, otherwise the entry is a no-op. Consider using/app(if the binary resides there) or removing this line since you copybenderinto/binlater.
Xeratec
left a comment
Xeratec
left a comment
There was a problem hiding this comment.
Thanks for the contribution, overall looks very good just some small comments. Please also make sure to check the open TODOs. Once this is done, I can quickly check again and approve the PR.
And we could also add a PyPi badge in the Readme ;)
I guess we could use, after a first release
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/install.md (3)
28-28:⚠️ Potential issue | 🟠 MajorFix broken
exportcommand syntax.
export $C_INCLUDE_PATH:...is invalid and will fail in shells. Use an assignment form so users can actually set the include path.✅ Proposed fix
-export $C_INCLUDE_PATH:/usr/include:/usr/include/aarch64-linux-gnu:$C_INCLUDE_PATH +export C_INCLUDE_PATH=/usr/include:/usr/include/aarch64-linux-gnu:$C_INCLUDE_PATH
7-7:⚠️ Potential issue | 🟡 MinorFix user-facing typos.
“prove instructions” → “provide instructions”; “environemt” → “environment”.
✅ Proposed fix
-To install these various dependencies, we prove instructions below, and a `Makefile` setup. +To install these various dependencies, we provide instructions below, and a `Makefile` setup.-The Makefile expects the environemt variable `CMAKE` to be defined. +The Makefile expects the environment variable `CMAKE` to be defined.Also applies to: 50-50
16-18:⚠️ Potential issue | 🟡 MinorRemove
cargofrom theapt installcommand in docs/install.md
Rust tooling has been removed, socargois no longer required.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 147: Update the capitalization of "PyPi" to "PyPI" in the changelog entry
that currently reads "Update `README.md` to remove reference to NVIDIA's PyPi
index." Locate the exact string "PyPi" in CHANGELOG.md and replace it with
"PyPI" so the entry reads "Update `README.md` to remove reference to NVIDIA's
PyPI index."
- Around line 35-38: Rewrite the CHANGELOG entries to use third-person wording
and correct the capitalization of "PyPI": replace first-person phrases like "I
created a release of Banshee" and "I bumped the onnx-graphsurgeon version" with
third-person/neutral phrasing (e.g., "A release of Banshee was published..." and
"The onnx-graphsurgeon version was bumped..."), change "PyPi" to "PyPI"
everywhere, and tidy the sentence about "_export_graph assigns their export type
to the tensors before export" to clear grammar (e.g., "The _export_graph
function assigns export types to tensors before export"); preserve the factual
content and references such as "publish.yml action" and the Banshee release
link.
In `@Container/Dockerfile.deeploy`:
- Around line 45-54: The Dockerfile installs different Bender artifacts per
TARGETPLATFORM causing a version/source mismatch: amd64 uses v${BENDER_VERSION}
from pulp-platform while arm64 fetches a 0.28.3-rc1 fork from Xeratec; change
the arm64 branch in the multi-platform conditional (the block checking
TARGETPLATFORM) to fetch the same official release (use BENDER_VERSION and the
pulp-platform repo) or parameterize an ARM_BENDER_VERSION with clear docs, and
avoid RC/forked URLs unless explicitly required and documented; ensure the wget
URL, tarball name, and subsequent cp operations reference the same canonical
variables (BENDER_VERSION, UBUNTU_VERSION,
/app/install/bender-<version>-<arch>-linux-gnu-ubuntu${UBUNTU_VERSION}.tar.gz)
so both platforms use a consistent, officially-sourced binary or add a comment
explaining why the fork/RC is required.
🧹 Nitpick comments (1)
pyproject.toml (1)
25-25: The version constraint change is justified, but consider adding an upper bound for defensive coding.The codebase makes extensive use of
onnx_graphsurgeon(20+ imports across deployers, parsers, optimization passes) with standard Graph operations. No documented breaking changes exist between 0.3.x and 0.5.x; the only known change is packaging-related (TensorRT 10.1.0 no longer bundles it, requiring separate pip installation on standard PyPI). This distribution change supports the PR's objective of removing the NVIDIA PyPI dependency.While the version jump carries minimal documented compatibility risk, adding an upper bound (e.g.,
>=0.5.8,<0.6) remains a reasonable defensive practice to prevent unexpected breakage from future major releases not yet assessed against your codebase.
Xeratec
left a comment
Xeratec
left a comment
There was a problem hiding this comment.
LGTM.
I have an optional comment about bender, but we can also do this in another PR. Let me know what you prefer. I also assume you decided not to add the PyPi badge in the Readme, correct?
Please do not forget to adjust the Docker container in .github/workflows/_select-env.yml!
See 0a30548. I am building the Docker, then once it passes, I will revert the Docker link and merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Container/Dockerfile.deeploy`:
- Around line 45-52: The Dockerfile extracts bender tarballs into
/app/install/bender which isn't created: update the RUN block to create
/app/install/bender (e.g. mkdir -p /app/install/bender) before the conditional
wget/tar steps so tar extraction succeeds, and after each tar extraction delete
the downloaded archive (remove bender-${BENDER_VERSION}*.tar.gz) to avoid
leaving artifacts; ensure both the linux/amd64 and linux/arm64 branches perform
the same create-extract-clean sequence.
🧹 Nitpick comments (1)
README.md (1)
8-9: Fix the badge link formatting to avoid broken rendering.The docs badge/link is split by a newline inside the markdown, which can break rendering. Consider keeping the entire badge/link on one line (and optionally add a space between badges for clarity).
🛠️ Suggested fix
-[](https://pulp-platform.github.io/Deeploy/) + [](https://pulp-platform.github.io/Deeploy/)
This release includes improvements to the tiling and DMA code generation, new networks and operators, improved CI workflows, migration to PyTest, and support for PyPi package releases. Note: Since the release tag references the Docker container tagged with the release tag (ghcr.io/pulp-platform/deeploy:v0.2.1), the CI will initially fail. The Deeploy Docker image must be built after the release PR is merged and the CI restarted. ### List of Pull Requests - PyPi Package Deployment + Remove Banshee Dept [#154](#154) - PyTest Migration [#144](#144) - Update submodule `pulp-nn-mixed` [#145](#145) - Improve Profiling [#138](#138) - FP32 ReduceMean operator improvement [#137](#137) - Support for RMSNorm (Pow and Sqrt operators) [#136](#136) - Demo TinyViT compatibility with tiled Siracusa [#124](#124) - TinyViT on non-tiled Siracusa [#117](#117) - Support Fully Asynchronous DMAs [#114](#114) - Disallow shape inference [#128](#128) - Remove memory-aware node bindings [#123](#123) - Fix missing const's layout transformation and refactor NCHWtoNHWC passes [#122](#122) - Fix aliasing [#125](#125) - Support for 1D Autoencoder [#98](#98) - Refactor Logging for Improved Debugging [#115](#115) - Add reuse-tool as an SPDX license header linter [#113](#113) - Bug fixes, API Cleanup and Reduce Compiler Warning on PULP [#112](#112) - Fix PULP GEMM `batch` serialization [#109](#109) - Split CI Workflows by Platform and Task, Improve Formatting and Linting Reliability [#108](#108) - Refactor tiling code generation [#105](#105) - Change order of typeMatching entries [#68](#68) - Node Mangling to avoid duplication [#93](#93) - Prepare Post v0.2.0 Release [#104](#104) - Use Docker digests instead of arch-specific tags [#106](#106) - Fix `Unsqueeze` Op. when using ONNX opset 13 or higher (from attribute to input) [#119](#119) - Fix bias hoisting in generic GEMM with no bias [#126](#126)
2 participants
Added
The
publish.ymlaction to build a branch and push it to PyPi. The action is automatically triggered when a tag with the "v*" format is emitted.I created a release of Banshee so we don't need to rebuild it over and over. The
Makefilenow pulls that release depending on the platform.I bumped the onnx-graphsurgeon version such that we don't need to use NVIDIA's PyPi index anymore.
_export_graphassigns their export type to the tensors before export.TODO: Update changelogs
TODO: Update readme with new pyindex
Changed
PULP-Deeploytodeeploy-pulp.Fixed
install.mdto remove rust mention and fix test command.README.mdto remove reference to NVIDIA's PyPi index.PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.