ci: stop third-party artifact downloads reddening unrelated PRs - #1060
ci: stop third-party artifact downloads reddening unrelated PRs#1060jiwenc-nv wants to merge 1 commit into
Conversation
Every build-ubuntu matrix entry fetched the DepthAI device firmware and the CloudXR SDKs from the network, so a transient failure at artifacts.luxonis.com or NGC failed a job that had nothing to do with the change under test — 22 red Build Ubuntu runs in one afternoon, and main was hit too. Cache both. depthai-core's DownloadAndChecksum skips any file already on disk, so restoring its resources/ makes the fetch a no-op; the CloudXR scripts already prefer a local tarball in deps/cloudxr/ over NGC. Restore and save are split rather than using the combined cache action, because that one also saves on a failed job and CMake's file(DOWNLOAD) leaves a truncated file behind on error — a half-written .fwp would then satisfy the "already downloaded" check forever. The runtime SDK script also retries the NGC sweep, since curl's --retry never covers the 404 actually observed. The retry wraps the whole listed/unlisted sweep, not each request: a 404 on an individual remote name is how the script discovers which name was published, so per-request retry would cost a delay on every successful run. Refs #1049 Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe CI workflows now restore CloudXR SDK and DepthAI firmware artifacts from keyed caches and save them after successful completion. The CloudXR download script now selects NGC resources by release type, falls back from the listed resource when needed, removes partial files, and retries the complete download sweep up to three times with five-second delays. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change caches third-party CI artifacts and retries failed downloads to reduce unrelated build failures; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CI Workflow
participant Cache
participant Download Step
participant Artifact Source
CI Workflow->>Cache: Restore keyed artifact cache
Cache-->>CI Workflow: Cache hit or miss
CI Workflow->>Download Step: Download missing artifacts
Download Step->>Artifact Source: Request CloudXR or DepthAI resources
Artifact Source-->>Download Step: Return resources or failure
CI Workflow->>Cache: Save artifacts after successful completion
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes address issue Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ivany-nv
left a comment
ivany-nv
left a comment
There was a problem hiding this comment.
Approving. The reasoning holds up — I verified the two load-bearing claims against depthai-core at the pinned commit 8d32a1f rather than taking them on trust:
- Cache path is right. depthai-core's root
CMakeLists.txtsetsDEPTHAI_RESOURCES_OUTPUT_DIR "${CMAKE_CURRENT_BINARY_DIR}/resources", andsrc/plugins/oak/CMakeLists.txtadds it viaadd_subdirectory(${depthai_SOURCE_DIR} ${depthai_BINARY_DIR}), so that resolves tobuild/_deps/depthai-build/resources. - The truncation worry is real, not theoretical.
DownloadAndChecksumopens withif(EXISTS "${output}") ... return()and never re-verifies the checksum, so the split restore/save is justified. Firmware download failure ends inmessage(FATAL_ERROR "Aborting."), so Configure fails and the save is correctly skipped.
I also ran download_cloudxr_runtime_sdk.sh locally under bash 3.2 with a stubbed curl and reproduced your numbers: all-404 gives exit 1, 3 sweeps, 12 requests, 10s; the realistic happy path (-external 404s, plain name serves) is 4 requests and 0s added; no leftover files in deps/cloudxr on any failure path.
Two things worth a follow-up, neither blocking:
1. resources/ holds one file the "Configure succeeded ⇒ everything checksummed cleanly" comment doesn't cover. depthai also writes depthai-shared-commit-hash-<id>.txt there, and it sets DEPTHAI_SHARED_COMMIT_HASH_ENFORCE OFF under $ENV{CI} — only the bootloader one is ON. So a failed device-side hash download is non-fatal, DownloadAndChecksum leaves the partial ${output} on disk, Configure still succeeds, and the truncated .txt gets saved. From then on the EXISTS skip means it's never re-fetched and the host/device shared-commit check silently degrades to a spurious WARNING for the life of that key. That's the failure mode this PR set out to prevent, one file over. Narrowing the cached path to .../resources/*.tar.xz would close it.
2. The cross-PR benefit only arrives after this merges — worth a line in the description. Caches are branch-scoped: one written on jiwenc-nv/ci-stability is readable by that branch and its descendants, not by other PRs. Only default-branch caches are visible everywhere, so the 14-PR-stack scenario stays red until this lands on main and a main push run populates the keys. Otherwise someone may see the next PR still miss and conclude this didn't work.
Minor notes:
- The body says "keyed on the
DEPTHAI_COMMITpin"; the key ishashFiles('cmake/DepthAIVcpkgManifest.cmake'). Conservative in the safe direction and consistent with the existingCache vcpkgkey — just the wording. - Expect five "Unable to reserve cache" lines per arch on the first run, since the depthai key deliberately omits
build_type/python_versionand all six matrix entries race for it.actions/cache/savetreats that as informational, so it's harmless — just don't let it get read as a failure. - Cache budget is a non-issue: I measured CloudXR at 11.8 + 12.1 + 0.3 ≈ 24 MB/arch and the depthai firmware at ~6 MB/arch, so ~60 MB against the 10 GB repo limit. No risk of evicting the ccache/vcpkg entries.
- For the pinned 6.3.0 the listed
cloudxr-runtimeresource 404s on all four names and the unlisted one serves, so "3 attempts" is effectively three attempts at the unlisted resource. Free, since curl doesn't retry 4xx.
Nice touch that deps/cloudxr/.gitignore already ignores exactly *.tar.gz and *.tgz, matching the cached globs, and that nothing in the tree globs those tarballs version-agnostically — a stale cached tarball stays inert.
|
reviewed by yanziz-review-bot Summary Legend: 🚫 BLOCKER = merge-blocking | 💡 SUGGESTION = meaningful improvement | 🧹 NIT = precise small correction
Actionables (copy-paste-ready for implementation agents)
|
3 participants
CI reddens on things unrelated to the change under test: every
build-ubuntumatrix entry fetches the DepthAI device firmware and the CloudXR SDKs over the network, so one transient failure atartifacts.luxonis.comor NGC fails the job. Measured over one afternoon on a 14-PR stack: 22 failedBuild Ubunturuns, 48 failed jobs.mainis hit too.Cache both artifacts, and retry the one download that curl's
--retrycan't cover._deps/depthai-build/resourcesbefore Configure, keyed on theDEPTHAI_COMMITpin. Upstream'sDownloadAndChecksumskips any file already on disk, so a hit makes the fetch a no-op.deps/cloudxr/*.tar.gz|*.tgzinsidesetup-cloudxr-sdk, which bothbuild-ubuntuandtest-teleop-ros2use. Both scripts already prefer a local tarball over NGC.actions/cache: that one saves on failed jobs too, andfile(DOWNLOAD)leaves a truncated file behind on error — a half-written.fwpwould satisfy the "already downloaded" check forever.download_cloudxr_runtime_sdk.shretries the whole listed→unlisted sweep, not each request. A 404 on an individual remote name is how the script discovers the published name, so per-request retry would add delay to every successful run.fail-fastis left alone: it amplifies flakes but doesn't cause them, and disabling it burns runner minutes on genuine compile errors.Closes #1049
Testing
curl: all-404 → 3 sweeps, 12 requests, exit 1; first-candidate hit → 1 request, 0 s added.Summary by CodeRabbit
Performance
Bug Fixes