Repository navigation
Conversation
`examples/rl/dynamo_vllm_refit` in ai-dynamo/modelexpress was split into `dynamo_vllm_reshard_refit` and `dynamo_vllm_s3_delta_refit`, so the link in the weight-updates guide 404s. lychee reads that as a repo-wide failure, which turns every new PR red regardless of what it changes: the check still shows SUCCESS on older open PRs only because those runs predate the rename. The split also dated the sentence around the link. It said the example "does not qualify the S3 delta path", and there is now a dedicated example for exactly that path: its README publishes a full snapshot followed by two incremental snapshots over S3. Repointing the link alone would have left a claim that the neighbouring link disproves, so both halves are named for what they actually cover, and the honest caveat that neither qualifies every backend and topology stays. Signed-off-by: ayaangazali <ayaangazali@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: ayaangazali <ayaangazali.work@gmail.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 (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe ModelExpress refit documentation now separates full-weight trainer-to-inference resharding from S3 artifact delta refitting. It also states the qualification limits for both examples and keeps startup loading as a separate workflow. ChangesModelExpress refit documentation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The updated documentation links to the split workflows without introducing product or operational risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the broken link, the replacement paths, the documentation updates, and validation results. However, it does not follow the required template because it omits the "Where should the reviewer start?" section and the required "Related Issues" section with an issue link or confirmation that no related issue exists.
Comment |
|
/ok to test 3cdaae8 |
|
/ok to test 39e410d |
Remove the obsolete checkpoint source link and regenerate the operator references, adopt the split ModelExpress refit examples, and link the GB200 recipe to the release branch where it remains available. Adapted from #14353 and #14774; all replacement targets return HTTP 200. Signed-off-by: jain-ria <riajain@NVIDIA.com>
Remove the obsolete checkpoint source link and regenerate the operator references, adopt the split ModelExpress refit examples, and link the GB200 recipe to the release branch where it remains available. Adapted from #14353 and #14774; all replacement targets return HTTP 200. Signed-off-by: jain-ria <riajain@NVIDIA.com>
Summary
lycheeis failing on every new PR in this repo, and this is the one broken link causing it:ai-dynamo/modelexpresssplit that directory in two.examples/rlstill exists and now holdsdynamo_vllm_reshard_refitanddynamo_vllm_s3_delta_refit, so the old path is gone.Worth stating plainly because it looks intermittent: the check still reads SUCCESS on older open PRs such as #13888 and #14021 purely because those runs predate the rename. Any lychee run from now on fails, so this reddens new PRs regardless of what they change. I hit it on #14770, whose diff is two Rust files with no URLs.
The split also dated the sentence the link sits in. It claimed the example "does not qualify the S3 delta path", and there is now a dedicated example for that path: its README publishes a full snapshot
v1plus incremental snapshotsd2andd3over S3. Repointing the link on its own would have left a sentence that the adjacent link disproves, so each example is now named for what it covers, and the caveat that neither qualifies every backend and topology is kept.Validation
Both replacement paths were confirmed to exist before linking them, rather than inferred from the naming:
The description of each example is taken from its own README, not guessed: the reshard one "validates the complete inference-side lifecycle" for a full-weight version over NIXL, and the S3 delta one "exercises the CoreWeave-shaped weight-synchronization lifecycle" with a full snapshot followed by incremental deltas.
No occurrence of
dynamo_vllm_refitremains in the file.pre-commitover the changed file passes every hook exceptpytest-marker-report, which fails identically on a clean tree in this environment because vLLM is not installed.Docs-only change; no code paths touched.
🤖 Generated with Claude Code
Summary by CodeRabbit