Repository navigation
docs(cuda-memory): correct stale test_support cfg(test) claim (follow-up to #762's CI fix) - #818
Merged
Merged
Conversation
…le doc test_support's module doc still said the module was compiled 'behind the gpu-tests feature (and the crate's own cfg(test))'. That's stale: the gate is (and must be) feature = "gpu-tests" only. cfg(test) never usefully gated this module because cfg(test) is only set when this crate compiles as its own unit-test binary, not when tests/*.rs integration tests link it as a regular dependency (#807's root cause). Clarify the doc so it matches the actual gate in lib.rs and doesn't mislead future readers into thinking cfg(test) alone would expose test_support to tests/*.rs. No functional change: crates/onnx-runtime-cuda-memory/src/lib.rs and tests/vmm_kv_layout_residency_gpu.rs already carry the feature-only gate fix on origin/main (merged via PR #762, commit e9c0ab6), so this hotfix branch only needed the doc correction on top of latest main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #818 +/- ##
==========================================
- Coverage 78.20% 77.47% -0.74%
==========================================
Files 350 350
Lines 141311 141311
Branches 141311 141311
==========================================
- Hits 110519 109475 -1044
- Misses 26389 27437 +1048
+ Partials 4403 4399 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
PR #762 merged into
mainate9c0ab6a4and already carries the fix for theCI-wide compile break introduced by #807:
crates/onnx-runtime-cuda-memory/src/lib.rsgates
pub mod test_support;on#[cfg(feature = "gpu-tests")]only (previously#[cfg(any(test, feature = "gpu-tests"))]), and the one integration test thatuses
TestStream(tests/vmm_kv_layout_residency_gpu.rs) cfg-gates its importand body accordingly.
Root cause recap:
cfg(test)is only set when a crate compiles as its ownunit-test binary. Integration tests under
tests/*.rslink the crate as aregular (non-
--test) dependency, socfg(test)never applies to them. Theold gate
any(test, feature = "gpu-tests")therefore never actually exposedtest_supporttotests/vmm_kv_layout_residency_gpu.rsunlessgpu-testswaspassed explicitly — which is exactly what broke CI's honesty-checker base
(cuda-only, no
gpu-tests) build for 7 of 9 failed jobs on #762's own CI run.What this PR does
Since the functional fix already reached
mainvia #762, this PR is aminimal, standalone doc correction:
test_support.rs's module doc still saidthe module was compiled "behind the
gpu-testsfeature (and the crate's owncfg(test))" — stale wording that contradicts the actual (correct) gate inlib.rsand could mislead a future reader into re-introducing the same bug byassuming
cfg(test)alone helps here. Updated the doc to explicitly state thegate is
gpu-tests-only and explain whycfg(test)doesn't apply tointegration tests.
No functional/behavioral change. No
.squad/state included.Delta vs. Opus-approved fix (afbd2c1 on #762)
git diff afbd2c131 origin/main -- crates/onnx-runtime-cuda-memory/src/lib.rs crates/onnx-runtime-cuda-memory/tests/vmm_kv_layout_residency_gpu.rsis empty — main already has byte-identical content for both files via the
#762 merge. The only delta introduced by this branch beyond what Opus already
reviewed is the
test_support.rsdoc-comment correction (7 insertions / 3deletions), which was never part of the reviewed diff. Flagging this so a tiny
follow-up look is optional but available before merge.
Validation (local, CPU-only host, matching CI's commands)
cargo test --locked -p onnx-runtime-cuda-memory --features cuda --tests --no-run— compiles clean (base/no-gpu-tests config)cargo test --locked -p onnx-runtime-cuda-memory --features cuda,gpu-tests --tests --no-run— compiles clean (gpu-tests config)python .github/scripts/verify_cuda_test_honesty.py— passes: 416 tests/66 targets identical in both configs (416 ignored without gpu-tests; 412 fail-loud/4 ignored/0 passed with gpu-tests on this no-CUDA host)cargo clippy --locked -p onnx-runtime-cuda-memory --features cuda --all-targets -- -D warnings— cleancargo clippy --locked -p onnx-runtime-cuda-memory --features cuda,gpu-tests --all-targets -- -D warnings— cleancargo check --locked -p onnx-runtime-ep-cuda -p onnx-runtime-python --features onnx-runtime-python/cuda— cleancargo test --locked -p onnx-runtime-ep-cuda --features cuda --test capture_sync_contract— 1 passedcargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings— cleancargo fmt --all -- --check— cleanNotes
squad/ep-plugin-parity-cuda) temporarily carried this samecfg-gating fix directly on its own branch (to unblock its own CI) and has
now merged into
main, somain's CI should already be green going forward.This standalone branch exists per explicit request so the repo-wide CI
unblock has its own paper trail, separate from EP plugin export: run our Rust execution providers inside upstream ONNX Runtime #762's larger diff.