fix: Standardize namespace environment variable to DYN_NAMESPACE #2485
Conversation
qimcis
requested review from
nnshah1,
tanmayv25,
piotrm-nvidia,
ptarasiewiczNV,
ryanolson,
grahamking,
paulhendricks,
biswapanda,
tmonty12,
GuanLuo,
rmccorm4,
tedzhouhk,
alec-flowers,
kkranen,
jthomson04,
ishandhanani,
PeaBrane and
a team
as code owners
|
👋 Hi qimcis! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
external-contribution
WalkthroughRenamed the environment variable DYNAMO_NAMESPACE to DYN_NAMESPACE across deployment manifests and code. Updates affect sglang and vllm disagg_planner YAMLs, vllm args for endpoint namespace resolution, and planner defaults for namespace-derived configuration. Default values remain the same; no other logic or configuration changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/backends/vllm/src/dynamo/vllm/args.py (3)
116-116: Add backward-compatible fallback to DYNAMO_NAMESPACE (with deprecation warning).Prevents breakage if any deployment still sets DYNAMO_NAMESPACE. Safe to remove later after one release.
Apply this diff:
- namespace = os.environ.get("DYN_NAMESPACE", "dynamo") + namespace = os.environ.get("DYN_NAMESPACE") or os.environ.get("DYNAMO_NAMESPACE") or "dynamo" + if "DYNAMO_NAMESPACE" in os.environ and "DYN_NAMESPACE" not in os.environ: + logger.warning("DYNAMO_NAMESPACE is deprecated; please set DYN_NAMESPACE instead.")
118-123: Don’t override a user-provided --endpoint.Only synthesize the endpoint from DYN_NAMESPACE when the user hasn’t explicitly passed --endpoint.
Apply this diff:
- if args.is_prefill_worker: - args.endpoint = f"dyn://{namespace}.prefill.generate" - else: - # For decode workers, also use the provided namespace instead of hardcoded "dynamo" - args.endpoint = f"dyn://{namespace}.backend.generate" + if "--endpoint" not in sys.argv: + if args.is_prefill_worker: + args.endpoint = f"dyn://{namespace}.prefill.generate" + else: + # For decode workers, also use the provided namespace instead of hardcoded "dynamo" + args.endpoint = f"dyn://{namespace}.backend.generate"
72-75: Clarify the help text to reference DYN_NAMESPACE.Minor clarity tweak so users know where the namespace comes from.
Apply this diff:
- help="Enable prefill functionality for this worker. Uses the provided namespace to construct dyn://namespace.prefill.generate", + help="Enable prefill functionality for this worker. Uses DYN_NAMESPACE to construct dyn://<namespace>.prefill.generate",components/planner/src/dynamo/planner/defaults.py (1)
72-76: Planner defaults: add fallback to DYNAMO_NAMESPACE (deprecate) to ease rollout.If any deployments still set the old var, this keeps behavior intact while warning. The endpoint string will then correctly derive from the resolved namespace.
Apply this diff:
class SLAPlannerDefaults(BasePlannerDefaults): port = os.environ.get("DYNAMO_PORT", "8000") - namespace = os.environ.get("DYN_NAMESPACE", "vllm-disagg-planner") + namespace = os.environ.get("DYN_NAMESPACE") or os.environ.get("DYNAMO_NAMESPACE") or "vllm-disagg-planner" + if "DYNAMO_NAMESPACE" in os.environ and "DYN_NAMESPACE" not in os.environ: + logger.warning("DYNAMO_NAMESPACE is deprecated; please set DYN_NAMESPACE instead.") prometheus_endpoint = _get_default_prometheus_endpoint(port, namespace)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
components/backends/sglang/deploy/disagg_planner.yaml(1 hunks)components/backends/vllm/deploy/disagg_planner.yaml(1 hunks)components/backends/vllm/src/dynamo/vllm/args.py(1 hunks)components/planner/src/dynamo/planner/defaults.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1365
File: deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go:171-178
Timestamp: 2025-06-04T13:09:53.416Z
Learning: The `DYN_DEPLOYMENT_CONFIG` environment variable (commonconsts.DynamoDeploymentConfigEnvVar) in the Dynamo operator will never be set via ValueFrom (secrets/config maps), only via direct Value assignment. The GetDynamoDeploymentConfig method correctly only checks env.Value for this specific environment variable.
📚 Learning: 2025-06-04T13:09:53.416Z
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1365
File: deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go:171-178
Timestamp: 2025-06-04T13:09:53.416Z
Learning: The `DYN_DEPLOYMENT_CONFIG` environment variable (commonconsts.DynamoDeploymentConfigEnvVar) in the Dynamo operator will never be set via ValueFrom (secrets/config maps), only via direct Value assignment. The GetDynamoDeploymentConfig method correctly only checks env.Value for this specific environment variable.
Applied to files:
components/backends/sglang/deploy/disagg_planner.yaml
⏰ 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). (4)
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (2)
components/backends/vllm/deploy/disagg_planner.yaml (1)
16-17: DYN_NAMESPACE rename confirmed; no DYNAMO_NAMESPACE stragglers found
- Searched code, YAML, Python, and Go files—no occurrences of
DYNAMO_NAMESPACEremain.- All environment-variable usages now reference
DYN_NAMESPACEas intended.components/backends/sglang/deploy/disagg_planner.yaml (1)
14-15: Rename to DYN_NAMESPACE looks correct for sglang as well.This aligns with the operator’s injected env var and other code changes.
Reuse the repo-wide sweep script in my earlier comment to ensure there are no remaining DYNAMO_NAMESPACE references in sglang pathways.
| @@ -11,7 +11,7 @@ spec: | |||
| envs: | |||
| - name: DYNAMO_SERVICE_CONFIG | |||
There was a problem hiding this comment.
If we are looking for consistency, should we update all the DYNAMO_ envs?
There was a problem hiding this comment.
I think that it'd be a good change, l can go ahead and update all of them!
| @@ -71,7 +71,7 @@ def _get_default_prometheus_endpoint(port: str, namespace: str): | |||
|
|
|||
| class SLAPlannerDefaults(BasePlannerDefaults): | |||
| port = os.environ.get("DYNAMO_PORT", "8000") | |||
Overview:
Fixes inconsistency in namespace environment variable usage across components. The K8s operator injects DYN_NAMESPACE for all containers, but some components were still expecting DYNAMO_NAMESPACE.
Details:
Where should the reviewer start?
components/backends/vllm/src/dynamo/vllm/args.pycomponents/planner/src/dynamo/planner/defaults.pydeploy/cloud/operator/internal/dynamo/component_common.goRelated Issues:
Summary by CodeRabbit
Chores
Migration Notes