[ci]: cache FastVideo kernel builds in Modal#1562
Conversation
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a caching mechanism for building and reusing FastVideo kernel wheels in Modal CI to significantly speed up test execution. It adds a new script kernel_build_cache.py to manage the cache, updates build.sh to support building reusable wheels, and integrates this caching system into the Dockerfile and Modal test suites (pr_test.py and ssim_test.py). The review feedback identifies a potential crash in parallel CI environments due to FileExistsError not catching OSError (e.g., ENOTEMPTY) when renaming directories concurrently, and suggests using Path.glob instead of glob.glob for cleaner and more cross-platform path handling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
This PR has merge conflicts with the base branch. Please rebase: git fetch origin main
git rebase origin/main
# Resolve any conflicts, then:
git push --force-with-lease |
|
Thanks for the work here. This mostly lines up with #1523: the Modal PR/SSIM paths now share a kernel cache helper and cache volume, the key includes source plus Python/torch/CUDA/arch/build metadata, build.sh keeps local behavior unchanged unless --wheel-dir is passed, and the opt-in cross-container smoke gives useful coverage for the intended cache hit/miss flow. I have one code concern before approval: _store_cache_entry() catches every OSError from temp_entry.rename(cache_entry), removes the temp entry, and still returns cache_entry. The caller then always logs stored wheel cache entry. That is only safe for the concurrent-writer case if a valid cache entry actually exists afterward. For real Modal volume/store failures, this can make CI logs claim the wheel was cached when it was not. Please revalidate the target entry after the rename exception, and only swallow the error if the existing metadata/wheel match; otherwise fail or log a clear “built but not cached” state. Also, the current head is still failing Buildkite fastcheck: microscope-kernel-tests, microscope-transformer-tests, and microscope-encoder-tests. Since those lanes run through the pr_test.py path changed by this PR, they need to be investigated and resolved before merge. I’ll look into the Buildkite logs separately, but for now this should stay blocked on the failing tests. |
50e3f02 to
d0c883d
Compare
|
Do not merge it yet, the changes are not comprehensive. |
SolitaryThinker
left a comment
There was a problem hiding this comment.
Changes requested at exact head 5bcf430.
-
P1 — PR-controlled code can poison a shared executable cache.
pr_test.py:24,89-92,129-164mounts the same writablefastvideo-kernel-build-cachefor PR/fork code, executes the checked-out helper, and commits the volume.kernel_build_cache.py:253-284,353-358then trusts key/schema plus a wheel-shaped filename and installs that executable artifact in later jobs, including jobs carrying HF/W&B secrets. Modal volumes also commit in the background/on shutdown, so moving the explicit commit earlier is not a sufficient trust boundary. Please disable shared PR writes until a trusted base-controlled producer can publish artifacts that PR consumers mount read-only; untrusted PR code must not receive a cross-PR writable executable store. See Modal volume semantics. -
P1 — corrupt/partial entries are treated as hits and permanently wedge a key.
_cache_hit()does not verify a digest, size, or wheel archive; an install failure propagates without eviction/rebuild, and_store_cache_entry()refuses to replace an invalid existing entry. The unit fixtures attest_kernel_build_cache.py:98-104,145-164demonstrate that empty/plain-text.whlfiles count as valid. Persist and verify SHA-256 plus size, reject invalid wheel archives, quarantine/evict and rebuild on validation or install failure, then bump the schema. -
P2 — ABI-affecting inputs are absent from the key.
kernel_build_cache.py:190-225hashes only backend, resolved arch, andCMAKE_ARGSfrom the build inputs. It omits effectiveCFLAGS,CXXFLAGS,LDFLAGS, selected compiler versions,torch.__config__.show(), and PyTorch's CXX11 ABI bit. An exact-head probe changedCFLAGSwithout changing the key. Include those inputs and add an invalidation regression with the schema bump. -
P2 — the concurrency model does not match Modal snapshots. Parallel lanes do not observe another container's commit without reload, so same-key fanout still independently misses/builds and later commits last-write-wins. Use one trusted producer before fanout; if the intended win is only cross-run reuse, document that limit. A v2 volume improves commit scale but does not deduplicate same-key builds.
-
P2 — Fastcheck does not collect the new tests.
run_unit_test()'s explicit path list atpr_test.py:299-302omits both new modules, so the green unit lane did not run them. Add both paths and cover SSIM integration plus prebuilt/corrupt-hit recovery.
Evidence: exact-head focused tests passed 9/9; shell syntax, Python compilation, and git diff --check passed. A conflict-free virtual merge onto current main also passed the same 9 tests. The linked sequential Modal miss/store/hit smoke passes, but it does not exercise untrusted writers, corrupt entries, or concurrent snapshot behavior. There is no full-suite-passed context yet.
Summary
fastvideo-kernel/build.shto emit reusable wheels with--wheel-dir, while preserving Docker-prebuilt exact-match reuse.Fixes #1523
Validation
uvx pre-commit run --all-filespassed from/tmp/fastvideo_worktrees/issue_1523_fix_precommit_5bcf43at5bcf430842bb1ba4e1724673b9ac783150d5d05f.5bcf430842bb1ba4e1724673b9ac783150d5d05f:pytest fastvideo/tests/modal/test_pr_test.py fastvideo/tests/modal/test_kernel_build_cache.py -q(9 passed). Run: https://modal.com/apps/hao-ai-lab/main/ap-YYznw2S06t8JnquCXfg4ea5bcf430842bb1ba4e1724673b9ac783150d5d05f: first container reportedcache miss:and stored the wheel; second container reportedcache hit:and installed the cached wheel; cleanup removed the smoke cache namespace. Run: https://modal.com/apps/hao-ai-lab/main/ap-tnwg9bhPcTSs7Va36TcDce5bcf430842bb1ba4e1724673b9ac783150d5d05f: build #4280 passed, including microscope unit, kernel, transformer, encoder, and VAE lanes. Run: https://buildkite.com/fastvideo/ci/builds/4280pre-commitpassed for the PR head: https://github.com/hao-ai-lab/FastVideo/actions/runs/29089063252/job/863497479447ce54cbb9f2b9179712764e0161ac9ba38079e94.2d11d9f66d0566fca9fd0192114c7323b1419f3b.5bcf430842bb1ba4e1724673b9ac783150d5d05f.5bcf430842bb1ba4e1724673b9ac783150d5d05ffound no actionable findings.Remaining Risk
ssim_test.py::run_ssim_testswas not run.full-suite-passedgate configured by maintainers.Checklist
For model/pipeline changes, also check: