Skip to content

fix(planner): resolve aggregate workers by component type - #11578

Merged
sttts merged 1 commit into
ai-dynamo:mainfrom
waynehacking8:wayne/fix-11491-agg-worker-name
Jul 30, 2026
Merged

fix(planner): resolve aggregate workers by component type#11578
sttts merged 1 commit into
ai-dynamo:mainfrom
waynehacking8:wayne/fix-11491-agg-worker-name

Conversation

@waynehacking8

@waynehacking8 waynehacking8 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Resolve aggregate planner workers by their DGD component type instead of backend-specific names.
  • Treat the sole generic type: worker component as the planner's decode component, regardless of its name.
  • Keep the existing explicit-name fallback when multiple generic workers make type-only selection ambiguous.

This fixes #11491 and lets #9848 rename vLLM workers without carrying planner-side aliases for every historical name.

Validation

  • Current-main A/B through KubernetesConnector.validate_deployment: an aggregate DGD with an arbitrary worker name fails before this change and passes after it.
  • test_kubernetes_connector.py: 75 passed.
  • Ruff check and format check passed for both changed files.

Live-cluster startup and scaling were not run because they require a real aggregate deployment.

@waynehacking8
waynehacking8 requested review from a team as code owners July 12, 2026 11:58
@copy-pr-bot

copy-pr-bot Bot commented Jul 12, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@waynehacking8
waynehacking8 temporarily deployed to external_collaborator July 12, 2026 11:58 — with GitHub Actions Inactive
@waynehacking8
waynehacking8 temporarily deployed to external_collaborator July 12, 2026 11:58 — with GitHub Actions Inactive
@github-actions github-actions Bot added the fix label Jul 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi waynehacking8! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added external-contribution Pull request is from an external contributor planner labels Jul 12, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment thread components/src/dynamo/planner/monitoring/worker_info.py
Comment thread components/src/dynamo/planner/core/base.py Outdated
@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Aggregate worker resolution

Layer / File(s) Summary
Connector component-name overrides
components/src/dynamo/planner/connectors/global_planner.py, components/src/dynamo/planner/connectors/kubernetes.py, components/src/dynamo/planner/connectors/virtual.py, components/src/dynamo/planner/tests/unit/test_remote_planner.py
Connector worker-info APIs accept optional component names; Kubernetes forwards them through DGD service and model-name resolution, while virtual and remote connector behavior remain signature-compatible.
Planner decode-name propagation
components/src/dynamo/planner/core/base.py, components/src/dynamo/planner/monitoring/worker_info.py
Decode resolution uses agg_worker_k8s_name in aggregate mode with fallback to decode_worker_k8s_name, and passes the resolved name through validation, initialization, refresh, and connector lookup paths.
Aggregate deployment coverage
components/src/dynamo/planner/tests/unit/test_kubernetes_connector.py
Tests cover aggregate worker construction, explicit-name validation success and failure, DGD service resolution, and model-name extraction.
Diagnostics formatting
components/src/dynamo/planner/core/base.py
The predicted-requests-per-second conditional expression is reformatted without changing its gating logic.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning A Prometheus diagnostics formatting change in base.py is unrelated to the agg-worker resolution objective. Remove or split out the unrelated Prometheus diagnostics formatting change so this PR stays focused on the aggregate DGD fix.
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is missing the required Overview, Details, Where should reviewer start?, and Related Issues template sections. Rewrite the PR description to follow the template, adding the missing sections and a linked issue or confirmed no-issue checkbox.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes implement the aggregate-mode resolution fix described in #11491 across validation, model-name, service, and worker-info paths.
Title check ✅ Passed The title is concise and accurately describes the main change: aggregate worker resolution by component type.

Comment @coderabbitai help to get the list of available commands.

Comment thread components/src/dynamo/planner/core/base.py Outdated
@waynehacking8
waynehacking8 temporarily deployed to external_collaborator July 12, 2026 12:10 — with GitHub Actions Inactive
@waynehacking8

Copy link
Copy Markdown
Contributor Author

Applied in 77f8bba. GPU-count resolution now gets the same explicit names (dynamo-review-agent's catch), and resolve_worker_info forwards decode_component_name to the model-name fallback (Devin's first point -- in practice that path only fires when the DGD args carry no model at all, since get_worker_info's arg-parsing fallback already used the name, but forwarding it makes the residual case fail with the truthful error). Devin's second point actually cut the other way once I checked the recipes: the repo's own vllm agg examples are split between VllmWorker (agg_failover, agg_gms) and VllmDecodeWorker (agg, agg_router, agg_kvbm), so a single static name would regress one shape -- resolution now takes ordered candidates (agg name, then decode name), which also covers the sglang/trtllm agg recipes via their decode names.

@waynehacking8
waynehacking8 force-pushed the wayne/fix-11491-agg-worker-name branch from 77f8bba to cda929e Compare July 17, 2026 14:49
@waynehacking8
waynehacking8 requested review from a team as code owners July 17, 2026 14:49
@waynehacking8
waynehacking8 temporarily deployed to external_collaborator July 17, 2026 14:49 — with GitHub Actions Inactive
@waynehacking8

Copy link
Copy Markdown
Contributor Author

Rebased over #10767, which moved deployment bootstrapping out of core/base.py into the environment facade -- the candidate derivation now lives in PlannerEnvironmentImpl._decode_component_name (environment/base.py) and the connector-side threading is unchanged. Re-ran the planner unit suites against current main (identical pass/fail set to main in my environment) and updated the PR body to match the new structure.

@Jont828

Jont828 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

@waynehacking8 It seems like your PR overlaps with mine #9848. I'm shortening the vllm renames but have a problem with making it backwards compatible. Maybe we can try to collab on getting both of these in together?

@waynehacking8

Copy link
Copy Markdown
Contributor Author

Happy to collab -- I think the two compose cleanly. This PR makes the planner's decode-role resolution try ordered name candidates read from the backend defaults, so your rename flows through automatically once the defaults say prefill/decode/worker, and the candidate list is also a natural home for the legacy fallback sttts asked about (try "worker" then "VllmWorker", so pre-rename DGDs keep resolving on the planner side -- it does not help the operator/override side of your change). If yours lands first I'll rebase mine over it.

@Jont828

Jont828 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

I think my PR is blocked on having backwards compatibility so yours would probably be a better candidate to merge first. I think we shouldn't rely on hard coded name lookups and instead have the selector choose based on the component type. That way, if we decide to change the name again, we'll need to maintain <new name>, worker, and VllmWorker too. IIRC the name lookup is a fallback behavior, so using it means that something fell through on the selection logic. Ideally the user should be able to name their prefill/decode/worker anything they want. The component type prefill or decode is a better indicator imo.

Do you want to take this? Or could I PR into your branch or something?

@Jont828

Jont828 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@waynehacking8 Also are you on the CNCF Slack? Could ping you there too in case we need to chat/brainstorm some more.

Aggregate v1beta1 DGDs expose one generic type: worker component, so exact backend-specific names make planner startup and scaling depend on recipe naming. Treat the sole generic worker as the decode component while keeping explicit-name disambiguation for legacy disaggregated deployments with multiple generic workers.

Fixes ai-dynamo#11491

Signed-off-by: WEI CHENG CHIU <waynehacking8@gmail.com>
@waynehacking8
waynehacking8 force-pushed the wayne/fix-11491-agg-worker-name branch from cda929e to 4229039 Compare July 24, 2026 01:12
@waynehacking8
waynehacking8 temporarily deployed to external_collaborator July 24, 2026 01:12 — with GitHub Actions Inactive
@waynehacking8 waynehacking8 changed the title fix(planner): resolve aggregate (mode=agg) DGDs via agg_worker_k8s_name fix(planner): resolve aggregate workers by component type Jul 24, 2026
@waynehacking8

waynehacking8 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@Jont828 Yes, I took this. I replaced the component name candidate plumbing with a type based fallback: a single generic type: worker now resolves as decode, while multiple generic workers still use the existing explicit name disambiguation. I rebased and force pushed the two file version; the connector suite passes 75/75, so #9848 no longer needs historical planner aliases.

@waynehacking8

Copy link
Copy Markdown
Contributor Author

@kaim-eng The planner fix is now reduced to a type-based fallback for the single generic worker, with 75/75 connector tests passing; this resolves #11491 without coupling the planner to backend-specific names. When you have time, could you review the rebuilt head?

@Jont828

Jont828 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@waynehacking8 Thanks for doing this! Btw, @sttts left a comment on my PR for renaming "VllmPrefillWorker" -> "prefill".

docs apparently still say VllmPrefillWorker. This matters for DGDR overrides. We should better make overrides silently support some normalization?

Does this component based matching work for overrides as well? Meaning if someone forgot to update the VllmPrefillWorker name, would it be silently dropped?

@waynehacking8

Copy link
Copy Markdown
Contributor Author

No. This PR only changes planner lookup after the DGD exists. DGDR overrides are merged earlier by exact spec.services key in apply_dgd_overrides. If the generated service is prefill, an override keyed by VllmPrefillWorker is skipped and a warning is logged. So it is not silent, but it is not normalized either. #9848 should update the override names or add compatibility in the override merge path; #11578 does not cover that.

@sttts

sttts commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

/ok to test 4229039

@sttts
sttts removed request for a team July 30, 2026 07:54
@sttts

sttts commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

/ok to test 4229039

@datadog-official

This comment has been minimized.

@sttts
sttts enabled auto-merge (squash) July 30, 2026 13:18
@sttts
sttts disabled auto-merge July 30, 2026 13:19
@sttts
sttts enabled auto-merge (squash) July 30, 2026 19:21
@sttts
sttts merged commit d66670f into ai-dynamo:main Jul 30, 2026
159 of 162 checks passed
pvijayakrish pushed a commit that referenced this pull request Aug 6, 2026
…#11578) (#12779)

Signed-off-by: WEI CHENG CHIU <waynehacking8@gmail.com>
Signed-off-by: Dan Gil <dagil@nvidia.com>
Co-authored-by: Wei-Cheng (Wayne) Chiu <waynehacking8@gmail.com>
Co-authored-by: Sachal <ssrivastavam@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor fix planner size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Planner: aggregate (mode=agg) DGDs not resolved via agg_worker_k8s_name in startup/validation/scaling

4 participants