opt(rfdetr): build Pillow-SIMD beside Pillow in the x86-64 CPU and GPU images - #2989
leeclemnet wants to merge 2 commits into
Conversation
|
👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely. 🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits. Warning 💸 The Claude reviewer bills in credits, not vibesAutomated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared. Draft when unsure, Ready when you mean it:
However you get there, arrive prepared:
Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.
|
d78fa58 to
d94870b
Compare
|
🤖 Claude review started at commit New commits are not auto-reviewed. Add the |
| # Pillow-SIMD (x86-64 only): the Pillow resample algorithm on SSE4 kernels, about 5x faster resizes. | ||
| RUN if [ "${TARGETPLATFORM}" = "linux/amd64" ]; then \ | ||
| pip3 uninstall -y pillow && \ | ||
| pip3 install --no-cache-dir --no-deps "pillow-simd @ git+https://github.com/uploadcare/pillow-simd.git@v12.2.0.post0" && \ |
There was a problem hiding this comment.
Medium (supply-chain / reproducibility): this installs and compiles third-party source into the official published images from a mutable git tag (@v12.2.0.post0). A tag can be force-moved and an upstream account/repo can be compromised, so a future build of the same Dockerfile could pull different code and bake it into a shipped Roboflow image — with no checksum or immutable ref to detect it. This also makes the build non-reproducible.
The rest of this repo already avoids exactly this: Dockerfile.onnx.gpu pins its from-source builds by immutable ref + checksum (GSTREAMER_COMMIT=88e3121…, FFMPEG_SHA256, OPENCV_SHA256). Please pin pillow-simd to the immutable commit SHA the tag currently points to (keep the tag in a comment for readability), e.g. pillow-simd @ git+https://github.com/uploadcare/pillow-simd.git@<commit-sha>.
Same line repeats in Dockerfile.onnx.cpu.dev, Dockerfile.onnx.gpu, Dockerfile.onnx.gpu.dev, and Dockerfile.onnx.cu13.gpu — please update all five.
Reviewed at HEAD: d94870b
There was a problem hiding this comment.
Pinned commit instead of tag in fbb023f.
Note: new pypi releases have been uploaded in the last 24h. 12.2.0.post0 is not there yet (12.1.1post0 is) but will continue to monitor and switch to standard pypi install once available. https://pypi.org/project/pillow-simd/#history
|
⏳ This PR is on hold pending your answers — the review will not advance to sign-off until the IMPORTANT question below is answered. IMPORTANT question
Please also confirm the Unanswered questions may keep this PR out of a release. Re-review is not automatic: new commits are not auto-reviewed — add the Reviewed at HEAD: d94870b |
|
Skills: review-packaging-ci, review-topic-backward-compat-and-versioning Review summary. Replaces the Pillow wheel with Open items (see the pinned action-item and the inline comment):
Maintainer notice (non-blocking): this touches Minor doubt (non-blocking): runtime shared-lib linkage for the source-built extension depends on Reviewed at HEAD: d94870b |
fbb023f to
a4c5e5f
Compare
The pillow version floor was raised from 12.2.0 to 12.3.0 in #2679. Pillow 12.3.0 fixed 13 security advisories that all affect 12.2.0, and pillow-simd 12.2.0.post0 carries the 12.2.0 code. As of today there is no pillow-simd 12.3.0 fork. Options:
Implemented option 2. Upgrade to pillow-simd 12.3.0post0 when available. |
a4c5e5f to
ce76fe2
Compare
…U images Pillow-SIMD tracks Pillow 12.2.0, which carries 13 advisories that 12.3.0 fixed, so it must not replace the wheel. The onnx.cpu, onnx.cpu.dev, onnx.gpu, onnx.gpu.dev and onnx.cu13.gpu images build v12.2.0.post0 (SSE4, pinned by commit) into /opt/pillow_simd on amd64 and verify that the wheel is intact and the build loads beside it. The Pillow floor stays at 12.3.0. inference_models imports that build under its own package name and uses only its Image.fromarray and resize, on an already decoded array, in the RF-DETR numpy preprocessor. Every decoder, filter and font path keeps running on the wheel. Standard Pillow stays in use when the directory is absent, when INFERENCE_MODELS_PILLOW_SIMD_PATH is empty, or when the build does not load. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ce76fe2 to
206d8cd
Compare
|
🤖 Claude review started at commit New commits are not auto-reviewed. Add the |
|
Skills: review-packaging-ci, review-inference-models-pkg, review-topic-prediction-integrity, review-topic-backward-compat-and-versioning, review-topic-test-hygiene Re-review at HEAD 206d8cd (redesign of the prior approach). Both previously-raised blockers are resolved in code:
Verified: the Maintainer notice (non-blocking). This is a functional change under Reviewed at HEAD: 206d8cd |
|
😎 PR passes the vibe-check and trust-me-bro verification. |
What does this PR do?
Builds Pillow-SIMD
v12.2.0.post0(MIT-CMU license) into the x86-64 CPU and GPU images beside the Pillow wheel, and lets the RF-DETR preprocessor use it for one call: the PIL resize that matches training. Pillow-SIMD is the Pillow resample algorithm with SSE4 kernels.Pillow-SIMD tracks Pillow 12.2.0, which carries 13 advisories that 12.3.0 fixed (heap out-of-bounds writes in
paste/crop,ImageCms,RankFilter; decompression-bomb bypasses in the GD, BDF and PCF loaders; DoS in PDF, EPS and JPEG2000). So it must not replace the wheel. The Pillow floor stays at>=12.3.0,PILstays the wheel, and onlyImage.fromarrayplusresizeon an already decoded array of known shape run on the SIMD build. No decoder, filter, font or PDF path ever reaches it.Changes:
Dockerfile.onnx.cpu,.cpu.dev: onlinux/amd64,pip install --no-deps --target /opt/pillow_simdfrom the pinned commit, thendocker/scripts/verify_pillow_simd.py.Dockerfile.onnx.gpu,.gpu.dev,.cu13.gpu: the same onamd64; the multi-stage images copy/opt/pillow_simdinto the runtime stage. On other arches the directory exists and is empty.docker/scripts/verify_pillow_simd.py: asserts that the standard wheel is at least 12.3, that the SIMD build loads under an alias with its own extension module, and that a resize agrees with the wheel within one gray level. Standalone, because the CPU image installsinference-modelsfrom PyPI.inference_models/models/common/pillow_simd.py: importing it loads the build atINFERENCE_MODELS_PILLOW_SIMD_PATH(default/opt/pillow_simd) as packagePILSIMDand exposes itsImagemodule. The import raisesModuleNotFoundErrorwhen the directory is absent or the variable is empty, andImportErrorwhen the build does not load.try: from ...pillow_simd import Image/except ImportError: from PIL import Image, thenfromarrayandresizeas before. The resize is called on the image, not through torchvision, because aPILSIMDimage is not aPIL.Image.Imageinstance. Same PIL bilinear resize either way.Output differs from the wheel by at most one gray level on about 0.1% of pixels on downscales, and is bit-exact on a no-op resize. With the directory absent or the variable empty the output is bit-exact with today's code.
Measured on
lee-t4-dev-cu128(T4, Xeon 2.3GHz),rfdetr-smallTRT, 2048×1371 chess image,pre_processmedian of 25, mean of two rounds in one session:pre_processrawpre_process512With the directory absent, or
INFERENCE_MODELS_PILLOW_SIMD_PATHempty, the output is bit-exact with main and the time matches main.Combined effect with #2988. That PR moves the channel swap after the resize, which removes a strided full-size copy that sits in front of the resize this PR speeds up. Each alone saves 14ms and 17ms of
pre_processon this image. Together they save 31ms, from 38.7ms to 7.9ms, and the server time on raw images drops from about 67ms to 35ms. The two PRs are independent and merge in either order; both touch the tail of_pre_process_numpy, so the second to merge takes a small conflict.Deploy note. The GPU images build
inference_modelsfrom the repo, so they get the loader with the image. The CPU image installsinference-modelsfrom PyPI, so its RF-DETR path keeps the wheel until the nextinference-modelsrelease carries the loader. The directory is harmless in the meantime.Related Issue(s): #2988
Type of Change
Testing
I have added/updated tests for this changeno test update neededTest details: see T4 latency measurement above
Workflow runs dispatched on this branch at
206d8cdbb(they run only onmainotherwise):USE_INFERENCE_MODELStrue and false)USE_INFERENCE_MODELStrue and false)Checklist
Additional Context
Unit tests in
tests/unit_tests/models/common/test_pillow_simd.pycover the import errors when the path is absent, empty, or fails to load, the alias load beside standard Pillow, and that the RF-DETR preprocessor binds to the build and falls back with equal output. They use a fake package, so they run on any CI box.tests/unit_tests/conftest.pypins the path to empty, so the RF-DETR exactness tests compare against standard Pillow on a host that has a build under/opt.The fork has no 12.3-based commit:
simd/masteris 12 commits on top of upstream12.2.0and 343 behind12.3.0, and its12.3.0tag is upstream Pillow's tag with no SIMD code. When the fork releases a 12.3-based build, the alias stays valid and the pin moves.🤖 Generated with Claude Code