ci(snapshot): gate DynamoCheckpoint CI on framework snapshot paths - #12037
Conversation
WalkthroughSnapshot CI detection now distinguishes vLLM, SGLang, and TRT-LLM changes, exposes corresponding outputs, validates filter behavior, and routes framework-specific build, deployment, testing, and cleanup jobs. ChangesSnapshot CI routing
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/test-filters.js:
- Around line 191-199: Add a test-filter matrix entry in the surrounding cases
of .github/scripts/test-filters.js for
components/src/dynamo/sglang/tests/test_sglang_snapshot.py, asserting
snapshot_sglang is true while sglang, snapshot, snapshot_vllm, and
snapshot_trtllm are false, consistent with the existing SGLang snapshot.py case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a10cde60-c696-4500-96a6-8b28371d6377
📒 Files selected for processing (5)
.github/FILTERS.md.github/actions/changed-files/action.yml.github/filters.yaml.github/scripts/test-filters.js.github/workflows/pr.yaml
|
🎯 Code Coverage (details) 🔗 Commit SHA: a9ce30d | Docs | Datadog PR Page | Give us feedback! |
|
|
|
@nv-tusharma Addressed in 36b90ca: removed the nonexistent vLLM/SGLang |
36b90ca to
08664e9
Compare
08664e9 to
dad6b09
Compare
|
/ok to test dad6b09 |
dad6b09 to
3c2df77
Compare
|
/ok to test 3c2df77 |
3c2df77 to
ae16d63
Compare
|
/ok to test ae16d63 |
Framework snapshot.py and test_*_snapshot.py only matched backend filters, so DynamoCheckpoint deploy jobs were skipped. Add per-framework snapshot filters and wire each suite to its own paths. Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
ae16d63 to
a9ce30d
Compare
|
/ok to test ae16d63 |
@galletas1712, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test a9ce30d |
Summary
snapshot_vllm/snapshot_sglang/snapshot_trtllmCI filters for each backend'ssnapshot.pyand TRT-LLM's existingtest_trtllm_snapshot.py.pr.yaml, so a framework-local snapshot change runs that backend's suite instead of being skipped.snapshotfilter for agent/operator/deploy paths (still runs all three frameworks)..github/scripts/test-filters.js.Fixes the gap seen on #11744, where vLLM/TRT-LLM snapshot warmup changes triggered backend unit tests but skipped all DynamoCheckpoint jobs.
Validation
cd .github/scripts && npm test— 26/26 filter pattern tests passedsnapshot_vllm/snapshot_trtllmonly if those files change; this PR changes filters/workflows socore/ciwill also match)Summary by CodeRabbit
CI Improvements
Documentation
Tests