Skip to content

test(router): add GMS placement E2E coverage#10484

Merged
hutm merged 0 commit into
review/gms-v2-latehost-09-3-router-event-plane-gms-d2d-worker-publishfrom
review/gms-v2-latehost-09-4-router-event-plane-gms-d2d-e2e
Jun 29, 2026
Merged

test(router): add GMS placement E2E coverage#10484
hutm merged 0 commit into
review/gms-v2-latehost-09-3-router-event-plane-gms-d2d-worker-publishfrom
review/gms-v2-latehost-09-4-router-event-plane-gms-d2d-e2e

Conversation

@hutm

@hutm hutm commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Stacked PR in the GMS-managed KV review train.

Base: review/gms-v2-latehost-09-3-router-event-plane-gms-d2d-worker-publish
Head: review/gms-v2-latehost-09-4-router-event-plane-gms-d2d-e2e

Refs #10457 #10458 #10450

This PR is intentionally scoped to one commit in the train so reviewers can inspect the GMS KV ownership, engine integration, Bulwark, host-tier, router, and docs work incrementally.

@hutm hutm requested review from a team as code owners June 9, 2026 05:07
@copy-pr-bot

copy-pr-bot Bot commented Jun 9, 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.

Comment thread tests/router/e2e_harness.py Outdated
Comment thread tests/router/helper.py Outdated
Comment thread tests/router/test_router_e2e_with_trtllm.py Outdated
@hutm hutm force-pushed the review/gms-v2-latehost-09-3-router-event-plane-gms-d2d-worker-publish branch from 17939aa to 0878c27 Compare June 9, 2026 14:07
@hutm hutm requested review from a team as code owners June 9, 2026 14:07
@hutm hutm force-pushed the review/gms-v2-latehost-09-3-router-event-plane-gms-d2d-worker-publish branch 2 times, most recently from 00c46ef to fef342b Compare June 9, 2026 14:19
@hutm hutm changed the title router: add GMS placement E2E coverage test(router): add GMS placement E2E coverage Jun 9, 2026
@github-actions github-actions Bot added the test label Jun 9, 2026
@hutm hutm force-pushed the review/gms-v2-latehost-09-4-router-event-plane-gms-d2d-e2e branch from 2d02bbf to 5b40a9c Compare June 9, 2026 14:58

from tests.gpu_memory_service.common.gms import GMSServer

devices = os.environ.get("DYNAMO_ROUTER_E2E_GMS_DEVICES", "0,1")

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.

When GMS is enabled, the harness starts servers on devices 0,1 by default even for gpu_1/single-GPU tests, so those tests can fail by trying to open GPU 1 on a one-GPU runner. Fix: default the GMS device list to the resolved single test GPU unless DYNAMO_ROUTER_E2E_GMS_DEVICES is explicitly set.

🤖 AI Fix

In tests/router/e2e_harness.py, update maybe_router_gms_servers so the unset DYNAMO_ROUTER_E2E_GMS_DEVICES default is str(int(os.environ.get("DYNAMO_ROUTER_E2E_GPU_START_INDEX", "0"))) instead of "0,1", leaving explicit multi-device coverage controlled by DYNAMO_ROUTER_E2E_GMS_DEVICES.

),
"context_length": _env_int("DYNAMO_ROUTER_E2E_SGLANG_CONTEXT_LENGTH", 1024),
"disable_cuda_graph": _env_bool(
"DYNAMO_ROUTER_E2E_SGLANG_DISABLE_CUDA_GRAPH", False

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.

The SGLang default now leaves CUDA and piecewise CUDA graphs enabled, but the same-GPU two-worker tests rely on disabling them to avoid the documented capture-time VRAM spike. Fix: keep the default disabled and allow opt-in via the env override.

🤖 AI Fix

In tests/router/test_router_e2e_with_sglang.py, change the SGLANG_ARGS["disable_cuda_graph"] default passed to _env_bool("DYNAMO_ROUTER_E2E_SGLANG_DISABLE_CUDA_GRAPH", ...) from False to True.

@hutm hutm force-pushed the review/gms-v2-latehost-09-3-router-event-plane-gms-d2d-worker-publish branch from fef342b to 46b3e7e Compare June 9, 2026 18:03
@grahamking

Copy link
Copy Markdown
Contributor

+58,833 -1,057

I think you accidentally included all of the stacked PRs.

@grahamking grahamking 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.

Include everything, I think this is a mistake needs rebase maybe. 60k lines.

@hutm hutm closed this Jun 29, 2026
@hutm hutm force-pushed the review/gms-v2-latehost-09-4-router-event-plane-gms-d2d-e2e branch from 5b40a9c to 8bc2ae7 Compare June 29, 2026 17:30
@hutm hutm merged commit 8bc2ae7 into review/gms-v2-latehost-09-3-router-event-plane-gms-d2d-worker-publish Jun 29, 2026
@hutm hutm force-pushed the review/gms-v2-latehost-09-3-router-event-plane-gms-d2d-worker-publish branch from 46b3e7e to 1df43fa Compare June 29, 2026 17:30
@hutm hutm deleted the review/gms-v2-latehost-09-4-router-event-plane-gms-d2d-e2e branch June 29, 2026 17:30
@hutm

hutm commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Historical review record preserved. GitHub cannot retarget this merged stacked PR; its current replacement is #11053 (position 10/18). No historical comments or reviews are being deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/XS test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants