feat(RL): add worker discovery endpoint - #9681
Merged
Merged
Conversation
biswapanda
force-pushed
the
bis/rl-workers-admin-routes
branch
from
May 18, 2026 16:07
84f0357 to
d4a15b6
Compare
biswapanda
force-pushed
the
bis/rl-workers-discovery
branch
from
May 18, 2026 16:49
df75f6b to
0d67a1b
Compare
dynamo-ops
approved these changes
May 18, 2026
biswapanda
force-pushed
the
bis/rl-workers-admin-routes
branch
from
May 18, 2026 16:51
d4a15b6 to
4d18d72
Compare
biswapanda
force-pushed
the
bis/rl-workers-discovery
branch
from
May 18, 2026 16:51
0d67a1b to
f28a1c2
Compare
biswapanda
force-pushed
the
bis/rl-workers-discovery
branch
from
May 18, 2026 16:56
f28a1c2 to
ecae356
Compare
biswapanda
force-pushed
the
bis/rl-workers-admin-routes
branch
2 times, most recently
from
June 6, 2026 21:01
31f9cbc to
9d1d61c
Compare
biswapanda
force-pushed
the
bis/rl-workers-discovery
branch
from
June 6, 2026 21:01
ecae356 to
8f0be53
Compare
biswapanda
force-pushed
the
bis/rl-workers-admin-routes
branch
from
June 6, 2026 21:19
9d1d61c to
3ca7ddd
Compare
biswapanda
force-pushed
the
bis/rl-workers-discovery
branch
from
June 6, 2026 21:19
8f0be53 to
bbe4095
Compare
biswapanda
force-pushed
the
bis/rl-workers-admin-routes
branch
from
June 7, 2026 07:28
3ca7ddd to
489c4a0
Compare
biswapanda
force-pushed
the
bis/rl-workers-discovery
branch
from
June 7, 2026 07:28
bbe4095 to
57ff99b
Compare
biswapanda
force-pushed
the
bis/rl-workers-discovery
branch
from
June 9, 2026 18:27
2d223e4 to
e487c9e
Compare
Contributor
WalkthroughThis PR introduces a new ChangesRL Worker Discovery Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/llm/src/http/service/service_v2.rs`:
- Around line 477-508: The RL discovery listener currently only logs bind
failures in spawn_rl_listener_if_configured, leaving the main HTTP service
running; change the Err(e) branch so the process fails fast: after logging the
error (address = %rl_addr, error = %e) call std::process::exit(1) (or otherwise
trigger global shutdown) so the service does not start silently without RL
discovery; update the Err branch inside spawn_rl_listener_if_configured (the
match on tokio::net::TcpListener::bind) to perform the exit/shutdown
immediately.
In `@lib/rl/src/lib.rs`:
- Around line 242-243: The call to wait_for_client_targets currently hardcodes
Duration::from_secs(5) and ignores its result, so the configured request_timeout
isn't respected and timeouts are swallowed; update the calls (the one at
wait_for_client_targets(&client, &[target.instance_id],
Duration::from_secs(5)).await and the similar block at lines ~276-287) to use
the configured request_timeout Duration (e.g., request_timeout variable or
client.request_timeout) instead of 5s, await the Result and propagate or return
an Err on timeout rather than discarding it, and adjust call sites/signatures if
needed so wait_for_client_targets returns a Result that callers check and handle
consistently.
In `@tests/rl/test_worker_discovery.py`:
- Around line 25-26: The test is currently marked as pre_merge
(pytest.mark.pre_merge) but is an expensive e2e with `@pytest.mark.timeout`(900);
change the marker to pytest.mark.post_merge on the test decorator(s) (replace
pytest.mark.pre_merge with pytest.mark.post_merge) or, if it truly must remain
pre_merge, add an explicit justification comment above the decorator(s)
explaining why this long-running test is critical for pre-merge gating; apply
the same change or justification to the other occurrences around the block
referenced (the markers at the later lines 210-213).
- Around line 90-93: _prepare_log_dir currently creates/deletes a relative
directory under the repo (f"{request.node.name}_{suffix}") which can collide and
writes into the tree; replace that with pytest's temporary directory by
obtaining the tmp_path fixture (use request.getfixturevalue("tmp_path") if you
don't want to change the call signature) and create/return a subdirectory under
it (e.g., tmp_path / f"{request.node.name}_{suffix}"), removing the manual
rmtree and returning the absolute path string so logs are written to
pytest-managed temp locations instead of the repository.
- Around line 24-31: The pytest markers on the pytestmark list are missing the
required VRAM and vLLM KV sizing markers; update the pytestmark list (the
variable pytestmark that decorates this test and references TEST_MODEL) to
include pytest.mark.profiled_vram_gib(N) with the correct GiB value for
TEST_MODEL and the vLLM KV sizing marker pytest.mark.vllm_kv_sizing(...) (or the
project’s canonical vLLM KV marker) so CI schedulers can place it safely.
In `@tests/rl/weight_update_worker.py`:
- Around line 33-35: Replace the runtime assert in update_weights_from_path with
explicit validation: in the update_weights_from_path(self, weight_path: str)
method check if not weight_path (and optionally if not isinstance(weight_path,
str)) and raise a clear exception (e.g., raise ValueError("weight_path must be a
non-empty string")) instead of using assert, then proceed with the existing
logic and return None as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50522a81-a15a-43e2-8ef9-051af1b95cd9
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locklib/bindings/kvbm/Cargo.lockis excluded by!**/*.locklib/bindings/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomllib/llm/Cargo.tomllib/llm/src/entrypoint/input/http.rslib/llm/src/http/service/openai.rslib/llm/src/http/service/service_v2.rslib/rl/Cargo.tomllib/rl/src/lib.rstests/rl/__init__.pytests/rl/test_worker_discovery.pytests/rl/weight_update_worker.py
…routes/models, add unit tests
…nd-to-end deadline)
biswapanda
enabled auto-merge (squash)
June 9, 2026 20:08
jthomson04
approved these changes
Jun 9, 2026
yao531441
pushed a commit
to yao531441/dynamo
that referenced
this pull request
Jun 10, 2026
Signed-off-by: Yao, Qing <qing.yao@intel.com>
nv-tusharma
added a commit
that referenced
this pull request
Jun 11, 2026
test_rl_worker_discovery_and_engine_admin_routes launches its vLLM worker via a
hardcoded command that omitted the GPU-memory budget every other vLLM e2e test
injects. Under the parallel GPU scheduler the worker is bin-packed onto a shared
GPU but ignored its _PROFILE_OVERRIDE_VLLM_KV_CACHE_BYTES budget, so vLLM fell
back to gpu_memory_utilization=0.9, demanded ~90% of the whole card, and crashed
during engine init ("Free memory ... less than desired GPU memory utilization")
with exit code 1 — surfacing as "Main server process exited with code 1 while
waiting for health check". 77.9% failure rate on main since #9681.
Route the worker command through build_gpu_mem_args("build_vllm_gpu_mem_args")
like the standard vLLM workers, so it honors the scheduler's
--kv-cache-memory-bytes budget under parallel runs and uses a conservative
--gpu-memory-utilization 0.4 fallback when run serially. The test already
declares requested_vllm_kv_cache_bytes(331_801_000); this makes the worker
actually respect it (tests/README.md documents this as a hard requirement).
Reproduced locally under simulated bin-packing (43 GiB held, ~3.5 GiB free):
the unfixed worker dies at the vLLM free-memory check (exit 1); with the fix the
engine initializes within its 0.31 GiB KV budget and proceeds past the memory gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Broduker
pushed a commit
to Broduker/dynamo
that referenced
this pull request
Jun 12, 2026
Signed-off-by: shenls <shenlinshan@kanzhun.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds the frontend worker discovery surface for RL frameworks. This is stacked on the worker-side RL admin routes PR and exposes only
GET /v1/rl/workers; admin execution remains direct-to-worker throughworkers[*].system_urland/engine/<route>.Changes
dynamo-rlcrate with read-onlyGET /v1/rl/workers.dyn://<namespace>.<component>.rlinstances and query them with{"method":"routes"}.DYN_ENABLE_RL=trueandDYN_RL_PORTdefaulting to8001./engine/*admin calls.Validation
cargo check -p dynamo-rlcargo check -p dynamo-llm.venv/bin/python -m pytest tests/rl/test_worker_discovery.py::test_rl_worker_discovery_and_engine_admin_routes -q/home/biswaranjanp/dev/rl/work/bis-dev/may-18/fullweight-fs-e2e-simpleNotes
supersedes #9382
Summary by CodeRabbit
Release Notes
New Features
/v1/rl/workersendpointTests