fix: set frontend defaults #2484
Conversation
mohammedabdulwahhab
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
biswapanda,
grahamking,
hhzhang16,
hutm,
ishandhanani,
jthomson04,
julienmancuso,
kkranen,
nnshah1,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
tanmayv25,
tedzhouhk and
tmonty12
as code owners
WalkthroughThis change removes explicit Frontend startup commands from backend deployment YAMLs, introduces env-driven defaults for frontend CLI flags, and updates the operator to set framework-specific working directories, default frontend command/args, and HTTP port env. It threads BackendFramework through pod-spec generation and adjusts tests to reflect these defaults. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant PodSpecGen as GenerateBasePodSpec
participant FrontendContainer as Frontend Container
participant FrontendProc as dynamo.frontend
Operator->>PodSpecGen: GenerateBasePodSpec(BackendFramework)
PodSpecGen->>FrontendContainer: Set WorkingDir (by framework), Command ["python3"], Args ["-m","dynamo.frontend"], Env DYNAMO_HTTP_PORT
FrontendContainer->>FrontendProc: Start with image entrypoint/command
FrontendProc->>FrontendProc: Parse args with defaults from env (DYNAMO_HTTP_PORT, DYNAMO_ROUTER_MODE)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions 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: 3
🧹 Nitpick comments (6)
components/frontend/src/dynamo/frontend/main.py (1)
95-97: Router-mode env default is fine; consider validating env on startup (optional)An invalid DYNAMO_ROUTER_MODE env defaults to RoundRobin in code paths below. If you prefer strictness, validate the env value and log/warn or error out early.
deploy/cloud/operator/internal/dynamo/component_frontend.go (1)
108-112: Env var added; align readiness probe with DYNAMO_HTTP_PORT (fallback-safe)You add DYNAMO_HTTP_PORT but the readiness probe still curls ${DYNAMO_PORT}. To avoid drift, use DYNAMO_HTTP_PORT with a fallback to DYNAMO_PORT.
Proposed readiness probe command (outside this hunk):
Command: []string{ "/bin/sh", "-c", "curl -s http://localhost:${DYNAMO_HTTP_PORT:-${DYNAMO_PORT}}/health | jq -e \".status == \\\"healthy\\\"\"", },This keeps behavior stable while preferring the new env.
deploy/cloud/operator/internal/dynamo/graph_test.go (4)
4309-4313: Avoid a potential panic: guard against empty containers sliceIf GenerateBasePodSpec ever regresses and returns an empty Containers list, this will panic. Add a quick guard as done in other tests in this file.
Apply this diff:
- // Check working directory + // Check working directory + if len(podSpec.Containers) == 0 { + t.Fatalf("GenerateBasePodSpec() returned no containers") + } if podSpec.Containers[0].WorkingDir != tt.wantWorkingDir { t.Errorf("GenerateBasePodSpec() workingDir = %v, want %v", podSpec.Containers[0].WorkingDir, tt.wantWorkingDir) }
4315-4322: Make command assertion slightly less brittle across imagesSome images may expose “python” vs “python3” as the interpreter. Keeping the args strict but relaxing the interpreter check improves portability without losing intent.
Apply this diff:
- // Check command and args - if !reflect.DeepEqual(podSpec.Containers[0].Command, []string{"python3"}) { - t.Errorf("GenerateBasePodSpec() command = %v, want %v", - podSpec.Containers[0].Command, []string{"python3"}) - } - if !reflect.DeepEqual(podSpec.Containers[0].Args, []string{"-m", "dynamo.frontend"}) { - t.Errorf("GenerateBasePodSpec() args = %v, want %v", - podSpec.Containers[0].Args, []string{"-m", "dynamo.frontend"}) - } + // Check command and args + if len(podSpec.Containers[0].Command) == 0 || !strings.Contains(podSpec.Containers[0].Command[0], "python") { + t.Errorf("GenerateBasePodSpec() command = %v, want interpreter starting with python*", + podSpec.Containers[0].Command) + } + if !reflect.DeepEqual(podSpec.Containers[0].Args, []string{"-m", "dynamo.frontend"}) { + t.Errorf("GenerateBasePodSpec() args = %v, want %v", + podSpec.Containers[0].Args, []string{"-m", "dynamo.frontend"}) + }
4226-4337: Optional: add a “noop/unknown” framework caseTo lock in the behavior that unknown frameworks don’t set a working dir, consider a negative case (BackendFrameworkNoop or an unknown string) that expects WorkingDir == "" and still uses python3 -m dynamo.frontend.
Happy to draft the additional table entry if you want it included in this PR.
4314-4322: Optional: also assert container name is “main”Other tests in this file rely on the convention that the primary container is named “main”. Asserting it here would catch regressions earlier.
You can append:
if podSpec.Containers[0].Name != "main" { t.Errorf("GenerateBasePodSpec() container name = %s, want main", podSpec.Containers[0].Name) }
📜 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 (9)
components/backends/sglang/deploy/agg.yaml(0 hunks)components/backends/vllm/deploy/agg.yaml(0 hunks)components/backends/vllm/deploy/disagg_router.yaml(1 hunks)components/frontend/src/dynamo/frontend/main.py(1 hunks)deploy/cloud/operator/config/rbac/role.yaml(0 hunks)deploy/cloud/operator/internal/dynamo/component_common.go(1 hunks)deploy/cloud/operator/internal/dynamo/component_frontend.go(2 hunks)deploy/cloud/operator/internal/dynamo/graph.go(2 hunks)deploy/cloud/operator/internal/dynamo/graph_test.go(4 hunks)
💤 Files with no reviewable changes (3)
- components/backends/sglang/deploy/agg.yaml
- deploy/cloud/operator/config/rbac/role.yaml
- components/backends/vllm/deploy/agg.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 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:
deploy/cloud/operator/internal/dynamo/graph_test.go
🧬 Code Graph Analysis (4)
deploy/cloud/operator/internal/dynamo/component_frontend.go (3)
deploy/cloud/operator/internal/dynamo/component_common.go (1)
ComponentContext(41-47)deploy/cloud/operator/internal/dynamo/graph.go (4)
BackendFramework(595-595)BackendFrameworkVLLM(599-599)BackendFrameworkSGLang(598-598)BackendFrameworkTRTLLM(600-600)deploy/cloud/operator/internal/consts/consts.go (1)
DynamoServicePort(11-11)
deploy/cloud/operator/internal/dynamo/component_common.go (1)
deploy/cloud/operator/internal/dynamo/graph.go (1)
BackendFramework(595-595)
components/frontend/src/dynamo/frontend/main.py (1)
lib/llm/src/local_model.rs (1)
default(61-78)
deploy/cloud/operator/internal/dynamo/graph.go (2)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go (1)
DynamoComponentDeploymentOverridesSpec(56-58)deploy/cloud/operator/internal/dynamo/component_common.go (1)
ComponentContext(41-47)
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/2484/merge) by mohammedabdulwahhab.
components/frontend/src/dynamo/frontend/main.py
[error] 83-92: Black formatting check failed. The pre-commit hook reformatted 'components/frontend/src/dynamo/frontend/main.py'; please commit the changes or re-run pre-commit to fix formatting.
⏰ 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: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (9)
deploy/cloud/operator/internal/dynamo/component_common.go (1)
41-47: Struct extension LGTMAdding BackendFramework to ComponentContext is consistent with downstream usage and enables framework-aware defaults. No issues spotted.
deploy/cloud/operator/internal/dynamo/component_frontend.go (3)
26-37: WorkingDir defaults look good; ensure context passes the graph’s backendThis mapping is correct. Note: it depends on ComponentContext.BackendFramework being the graph’s framework (vllm/sglang/trtllm), not “noop”. See my graph.go comment about ensuring the context uses the graph-level backend for non-worker components.
43-47: WorkingDir assignment LGTM (subject to correct context propagation)This is correct and harmless when empty. The only caveat is ensuring the context.BackendFramework is set for Frontend components (see graph.go comment).
48-51: Good default: run python3 -m dynamo.frontendSwitching from /bin/sh -c to an explicit python3 command is cleaner and avoids shell quoting issues. User overrides still work via ExtraPodSpec merge.
deploy/cloud/operator/internal/dynamo/graph_test.go (5)
1286-1289: LGTM: Frontend now exports DYNAMO_HTTP_PORT in Pod specAdding DYNAMO_HTTP_PORT to the frontend container’s Env aligns tests with the new frontend defaults and makes the port explicit at runtime. Sorting of envs later in the test ensures stability.
2027-2030: LGTM: Multinode SGLang frontend includes DYNAMO_HTTP_PORTGood expectation update. This keeps the test consistent with the frontend’s env-driven defaults.
2780-2783: LGTM: Multinode VLLM frontend includes DYNAMO_HTTP_PORTConsistent with the new frontend defaults. Looks good.
4226-4337: LGTM: Solid coverage for frontend defaults (working dir, command/args, env)Nice table-driven test validating framework-specific working dir, the default python3 -m dynamo.frontend entrypoint, and the DYNAMO_HTTP_PORT env.
4226-4337: ✅ No duplicateTestGenerateBasePodSpec_Frontendfound
A repository-wide grep shows only the single declaration indeploy/cloud/operator/internal/dynamo/graph_test.go:4226. There are no naming conflicts that would cause the build to fail.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Overview:
Defaults now set for the working dir/command/args of the Frontend component on a DynamoGraphDeployment:
Summary by CodeRabbit
New Features
Refactor
Tests
Chores