Repository navigation
fix: backport ZIP9 archive compatibility to Candle 0.10.2 - #1
Conversation
|
@coderabbitai review Please review the Candle 0.10.2 ZIP 9 maintenance backport, including decoded archive-name lookup, duplicate precedence, NPZ/PTH regression fixtures and propagation of non-not-found ZIP errors. |
|
Independent read-only source and workflow review at commit The reviewer checked decoded-name lookup, mixed/raw duplicate precedence, suffix aliases, ZIP-error propagation, fixture structure/CRC/value integrity and the contents-read CPU workflow. They also checked 9,330 duplicate/alias sequences with a read-only model. They inspected local passing logs but did not launch Rust tests or access GitHub CI independently. Author validation passed 219 core tests/doctests and 27 strict-deflate regressions on both Rust 1.88.0 and 1.98.1, macOS arm64. Ubuntu CPU/MSRV checks are running. The inherited CUDA job failed before execution because the upstream runner group |
|
Full unedited supplemental source-and-workflow review below, for exact reviewed commit Read-only review passes for the final source-and-CI patch. I found no blocking source or workflow defect. This report supersedes the earlier source-only reports. Reviewed base: Exact reviewed identities:
The saved complete patch matches the complete diff hash. Excluding the new workflow reproduces the previously reviewed source-only hash exactly. The final checkout is clean. The source correctly accommodates ZIP9’s raw-byte archive index. ASCII lookup retains a safe direct path. Non-ASCII lookup compares decoded metadata names and selects the greatest central-header offset, handling mixed encodings and interleaved raw duplicates without opening unrelated payloads. Enumeration preserves the first decoded-name position and last central-directory payload. Deduplication precedes Selective NPZ loading correctly maps only The naming differences are intentional: ZIP9 tries UTF-8 independently of the filename flag, then falls back to CP437. Invalid flagged UTF-8 uses CP437 instead of ZIP7’s lossy UTF-8; CP437 bytes forming valid UTF-8 use the UTF-8 name. Ordinary CP437 lookup remains supported. Rust 1.88 is declared in workspace metadata and inherited by core, matching both cached ZIP9.0.3 and ZIP9.0.4 metadata. The The new workflow is appropriately scoped:
Checkout follows standard GitHub behavior: PR jobs test the merge ref; push jobs test the pushed commit. Dependencies resolve afresh per job and remain locked within that job. Existing repository CI remains unchanged. I did not independently verify action-commit provenance through GitHub. Independent evidence includes the complete patch reads, passing diff checks, fixture structure/CRC/value inspection, 9,330 read-only duplicate-and-alias model comparisons, verification that all 13 NPZ fixtures are tracked, and inspection of the 27 regression definitions. I also inspected the provided local logs. The full-core and minimum-Rust logs each contain 219 passing tests and zero failures; the default archive, deflate archive and minimum-Rust deflate logs each contain 27 passing tests and zero failures. I did not launch those executions. Clippy remains red with 15 diagnostics; I independently verified that every cited source line equals its upstream-base line. That establishes unchanged diagnostic locations, not a passing baseline compilation. No GitHub CI execution is claimed. CUDA, Metal, alternate targets, encrypted archives and other compression backends remain outside this review’s validation. No files were written, external services accessed, workloads launched or report published. |
Candle 0.10.2 depends on yanked ZIP 7 releases, preventing fresh dependency resolution. This maintenance backport uses ZIP 9 without changing the Candle tensor APIs or backend features.
The NPZ and PyTorch readers use fallible decoded filenames and metadata-only lookup. Ordinary CP437 filenames work in eager, lazy and selective reads. Mixed encodings and raw duplicates retain first decoded-name position and last central-directory payload selection. Deduplication precedes
.npysuffix removal. Selective NPZ reads keep the missing-array error only forFileNotFound; other ZIP failures propagate.Compatibility limits are explicit:
Error::Zipcontains ZIP 9's error type. Code using the old concrete error type needs to be rebuilt.candle-coredeclares Rust 1.88, matching ZIP 9's requirement. The core CPU suite and explicit-deflate archive tests pass with Rust 1.88.0 and Rust 1.98.1 on macOS arm64.Based exactly on upstream Candle 0.10.2 commit
7c7a8c570e6a16ea24cc30a7501c8ffbbdb51680, targetingmaintenance/0.10.2. Upstream licensing and source metadata are preserved; no tags or releases are introduced.Validation on macOS arm64 with Rust 1.88.0 and Rust 1.98.1, resolving ZIP 9.0.4 through the
9.0.3semver requirement:cargo +1.98.1 test --offline --locked -p candle-core: 219 tests and doctests passed, including 27 archive regressions.CANDLE_TEST_EXPECT_DEFLATE=1 cargo +1.98.1 test --offline --locked -p candle-core --test zip_compatibility --features zip/deflate: 27 passed, with decompression required.CUDA, Metal and other optional backend builds remain unverified by these CPU tests. Consumers must pin an immutable reviewed revision and retain a reviewed Cargo lock; Candle core and Metal kernel overrides must use the same Git source revision to preserve crate identity.
A minimal contents-read workflow runs the core CPU suite and strict explicit-deflate regressions on Ubuntu with Rust 1.88.0 and Rust 1.98.1 for maintenance-branch PRs and pushes. Hosted Ubuntu CPU/MSRV results passed for both Rust versions (run
38888116434). Inherited full Ubuntu, Windows and macOS suites, every platform workspace check (including macOS Metal), Rustfmt and secret scanning also passed. The inherited Clippy and CUDA checks are red for the separately documented baseline/tooling reasons; this PR does not claim all checks are green.cargo clippy --locked -p candle-core --test zip_compatibility -- -D warningson Rust 1.98.1 reports 15 diagnostics in unchanged upstream source regions (npy.rs:121, quantized kernels andshape.rs:12). Each diagnostic line matches the upstream base verbatim. No lint suppression or unrelated source changes are included. This command did not pass; the upstream ZIP 7 baseline was not compiled with this toolchain.The actual consuming SIE change PR 765 merged as
175d226be52759f92555c9540b3564d8d5a6caa7after all 43 checks passed and CodeRabbit approved final consumer head623eae787cc9bcf11f3bfe17c880734bb73f2d3e. Its immutable source is exactly9d2d6c6c1831c3d6444faca8e687dce06bd6bbe0. The Linux Rust job passed the unchanged audit policy and locked worker validation; the production CUDA compatibility job built the locked Rust CUDA executable and checked driverless runtime dependencies. This is compile/container evidence, not GPU inference or numerical-quality evidence.CodeRabbit review was requested on this fork, but no bot review or approval was returned. The final independent source/workflow review and available hosted validation are recorded explicitly. Inherited CUDA failed before execution because its upstream runner group does not exist in this fork. Hosted stable Clippy reported 19 errors across 21 diagnostic locations; every diagnostic source line is identical to upstream
7c7a8c57. No lint suppression, unrelated source edit, audit exception, runner permission change or repository setting change was applied.