-
Notifications
You must be signed in to change notification settings - Fork 691
fix: update the tags for consistency and remove 0.4.1 refs #3058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update the tags for consistency and remove 0.4.1 refs #3058
Conversation
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
WalkthroughUpdated container image references across deployment YAMLs and READMEs for sglang, trtllm, and vllm backends. Tags were changed from specific or placeholder tags to “my-tag” and example registries updated to “my-registry”. No logic, configuration fields, or public APIs were altered. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/backends/trtllm/deploy/README.md (1)
144-144: LGTM on my-registry/my-tag example.Matches the convention introduced in this PR.
Small doc consistency nit: elsewhere in this README the sample image references
nvcr.io/nvidian/...(note the “nvidian” spelling). Consider correcting tonvcr.io/nvidia/...and/or using the same placeholder style to avoid mixed examples.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/backends/sglang/deploy/README.md(1 hunks)components/backends/trtllm/deploy/README.md(1 hunks)components/backends/trtllm/deploy/agg-with-config.yaml(2 hunks)components/backends/vllm/deploy/README.md(1 hunks)components/backends/vllm/deploy/agg.yaml(2 hunks)components/backends/vllm/deploy/agg_router.yaml(2 hunks)components/backends/vllm/deploy/disagg.yaml(3 hunks)components/backends/vllm/deploy/disagg_planner.yaml(5 hunks)components/backends/vllm/deploy/disagg_router.yaml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test - vllm
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (18)
components/backends/vllm/deploy/disagg_router.yaml (3)
16-16: Switched frontend image to my-tag — OK.If release tooling replaces
my-tag, confirm this file is included in that step.
30-30: Worker image tag standardized — OK.
47-47: Prefill worker image tag standardized — OK.components/backends/vllm/deploy/agg.yaml (2)
16-16: Frontend image tag updated — looks good.
27-27: Decode worker image tag updated — looks good.components/backends/vllm/deploy/agg_router.yaml (2)
16-16: Frontend image tag now uses my-tag — OK.
30-30: Decode worker image tag now uses my-tag — OK.components/backends/vllm/deploy/disagg_planner.yaml (5)
23-23: Frontend image tag standardized — OK.
54-54: Planner image tag standardized — OK.
94-94: Prometheus image tag standardized — OK.
117-117: Decode worker image tag standardized — OK.
142-142: Prefill worker image tag standardized — OK.Consider a follow-up to centralize the image tag (e.g., kustomize/helm values or CI substitution) to avoid drift across many files.
components/backends/vllm/deploy/disagg.yaml (2)
16-16: Frontend image tag updated — OK.
27-27: Decode worker image tag updated — OK.components/backends/vllm/deploy/README.md (1)
119-119: Approve — placeholder standardized; verification requiredLGTM; README now uses image: my-registry/vllm-runtime:my-tag and aligns with other backends.
Verification: the ripgrep run (rg -n "0.4.1|your-registry|your-tag" components/backends/vllm -S) returned no output in the provided execution, so absence of stragglers could not be confirmed. Re-run that command in the repository/CI and attach the output or confirm there are no matches.
components/backends/sglang/deploy/README.md (1)
95-95: LGTM for the README image change — update or confirm remaining sglang deploy image placeholders.README change approved. Search shows these files still contain the placeholder image "my-registry/sglang-runtime:my-tag":
- components/backends/sglang/deploy/agg.yaml
- components/backends/sglang/deploy/agg_router.yaml
- components/backends/sglang/deploy/agg_logging.yaml
- components/backends/sglang/deploy/disagg.yaml
- components/backends/sglang/deploy/disagg-multinode.yaml
- components/backends/sglang/deploy/README.md
No occurrences of "your-registry"/"your-tag"/"0.4.1" were found under components/backends/sglang; note that repo‑wide "0.4.1" references still exist (e.g., pyproject.toml, docs, Cargo.lock) — confirm whether those version references should be updated.
components/backends/trtllm/deploy/agg-with-config.yaml (2)
37-37: LGTM: tag standardized consistently.Both mainContainer image tags updated to my-tag as intended. No other manifest fields changed.
Also applies to: 53-53
37-37: Mark CI‑replaced image tag to avoid accidental deploymentsAdd an inline comment so the tag is obviously CI‑replaced in components/backends/trtllm/deploy/agg-with-config.yaml (also at the other occurrence). Apply this diff:
- image: nvcr.io/nvidia/ai-dynamo/tensorrtllm-runtime:my-tag + image: nvcr.io/nvidia/ai-dynamo/tensorrtllm-runtime:my-tag # replaced by CI at release timeVerification: repository search returned many "0.4.1" release refs and several "your-" placeholders that should be audited. Notable matches: deploy/helm/chart/Chart.yaml, container/Dockerfile.trtllm, docs/_includes/install.rst, docs/_includes/quick_start_local.rst, benchmarks/nixl/README.md, deploy/inference-gateway/README.md, and multiple recipes//deploy.yaml files referencing vllm-runtime:0.4.1. Remove or replace any literal my-tag/your-tag in manifests used for deployment, or confirm they are intentionally instructional.
Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a few refs to 0.5.0 as well -
https://github.com/ai-dynamo/dynamo/blob/main/examples/basics/kubernetes/Distributed_Inference/agg_router.yaml#L41
https://github.com/ai-dynamo/dynamo/blob/main/benchmarks/incluster/benchmark_job.yaml#L20
I see new commits, nvm!
nv-anants
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was working on something else and found a bunch under tests as well - https://github.com/ai-dynamo/dynamo/blob/main/tests/planner/scaling/disagg_planner.yaml#L119
The |
Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: Jason Zhou <jasonzho@nvidia.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: Jason Zhou <jasonzho@nvidia.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: Jason Zhou <jasonzho@nvidia.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
Overview:
Standardize the naming of
your-registryandyour-tagtomy-registryandmy-tagrespectively for ease of scriptable replacement at release time.Remove references to specific tags, in this case
0.4.1and0.5.0, in deference tomy-tagRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
relates to OPS-1182
Summary by CodeRabbit
New Features
Documentation
Chores