Skip to content

fix(generation): report no OpenAI server as an empty list - #3993

Open
yupengtang wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
yupengtang:fix/no-server-url-sentinel
Open

fix(generation): report no OpenAI server as an empty list#3993
yupengtang wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
yupengtang:fix/no-server-url-sentinel

Conversation

@yupengtang

@yupengtang yupengtang commented Sep 4, 2026

Copy link
Copy Markdown

What does this PR do ?

Makes all four generation backends report "no OpenAI server" the same way, as an empty list, and narrows dp_openai_server_base_urls to list[str].

Megatron already returns [] and filters its Nones out. vLLM, TRT-LLM and Dynamo returned a list of Nones instead. As #3977 says, the placeholder does not stand for anything: in those branches there is no server at all, so the length carries no information, but [None] is truthy, so "we have URLs" reads as true when there are none.

Changed to match Megatron:

backend before after
vLLM (sync engine) [None] []
vLLM (deferred load, sync engine) [None] []
TRT-LLM (no HTTP server) [None] * dp_size []
Dynamo (no token wrapper) [None] []

NemoGymConfig.base_urls and spinup_nemo_gym_actor already declare list[str], so this makes the producers match what the consumers were asking for rather than widening anything. The gap is not theoretical: #3367 had to add a cast(list[str], deferred_vllm.dp_openai_server_base_urls) in distillation.py to bridge it, which is the same review that prompted #3977. I left that cast in place, since it is correct either way, and the caller cleanup reads better as its own change.

One case the issue does not list

vLLM needed more than swapping the sentinel. A worker leaves base_url unset unless expose_http_server started a server (vllm_worker_async.py:104 assigns None, :206-210 only sets it under that flag), and get_reserved_url returns None unless a socket was reserved under the same flag. So async_engine: true with expose_http_server: false produced [None] * dp_size from the workers themselves, never reaching the guarded branch, which is a longer, still-truthy version of the same problem.

Why emptiness is decided for the whole list

Both new code paths test if not any(results) and return [], rather than filtering Nones out entry by entry. Two reasons:

  • expose_http_server is one config value shared by every worker, so the answer is genuinely all-or-nothing. A mixed list is not reachable today.
  • If one ever were, dropping entries would be the wrong response. GenerationFleetHealth indexes these URLs by shard and rejects a list whose length does not match shard_count. Filtering would turn a single degraded shard into a startup failure reporting base_urls has N entries for M shards: a length error that names neither the shard nor the cause. Keeping one entry per DP rank preserves the graceful path.

Issues

Closes #3977.

Usage

No config or API change. if not urls is now a correct emptiness test on every backend.

# before, on a sync vLLM engine
generation.dp_openai_server_base_urls   # [None]  -> truthy, len 1
# after
generation.dp_openai_server_base_urls   # []      -> falsy, len 0

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

Callers

I went through every reader before changing the producers. None needed edits, and none reads the length in the no-server case:

  • _shard_base_urls (setup.py:769) collapses an all-None list to None via if not any(urls); an empty list takes the same branch.
  • _maybe_start_generation_router filters falsy entries into backend_urls and raises when that is empty, so [None] and [] already produced the identical error.
  • NeMo-Gym spinup is reachable only behind the should_expose_http_server assert in nemo_rl/environments/nemo_gym.py, so the no-server list never gets there.
  • MegatronGeneration.verify_served_address only ever saw Megatron's list, which was already [].
  • No production caller indexes dp_openai_server_base_urls directly. The one len() call, grpo.py:1556, is a print on the deferred vLLM path. It now says Reserved 0 vLLM server URLs: [] instead of Reserved 1 vLLM server URLs: [None], which is the symptom the issue calls out.

Per the issue I left the now-redundant any(...) guards alone. They stay correct either way and are cleaner to remove separately.

Tests

Six added, plus the existing Dynamo assertion at test_dynamo_generation.py:167 updated from [None] to [].

tests/unit/models/generation/test_vllm_generation.py:

  • test_sync_engine_reports_no_openai_server_urls
  • test_async_engine_without_http_server_reports_no_urls: the case above, workers answering all Nones
  • test_async_engine_with_http_server_reports_served_urls: guards the happy path against over-collapsing
  • test_partially_reported_urls_keep_one_entry_per_rank: pins the whole-list rule so a later refactor cannot reintroduce per-entry filtering

tests/unit/models/generation/trtllm/test_trtllm_generation.py:

  • test_no_http_server_reports_an_empty_url_list
  • test_exposed_http_server_reports_served_addresses

Reverting each backend to its pre-fix version, the no-server tests fail and the served-address tests still pass:

# vllm_generation.py reverted
FAILED test_sync_engine_reports_no_openai_server_urls
FAILED test_async_engine_without_http_server_reports_no_urls
PASSED test_async_engine_with_http_server_reports_served_urls

# trtllm_generation.py reverted
FAILED test_no_http_server_reports_an_empty_url_list
PASSED test_exposed_http_server_reports_served_addresses

test_partially_reported_urls_keep_one_entry_per_rank also fails against an entry-filtering implementation, which is how I settled on the whole-list rule.

With the change, merged onto current main, the CPU-only tests in the touched suites pass, 29 before and 33 after, same deselect set:

pytest tests/unit/models/generation/test_vllm_generation.py   # GPU tests deselected
  main:      29 passed
  this PR:   33 passed

pytest tests/unit/models/generation/test_dynamo_generation.py              23 passed
pytest tests/unit/models/generation/trtllm/test_trtllm_generation.py       19 passed
pytest tests/unit/single_controller/test_setup.py -k 'url or shard or gym or router'   14 passed
pytest tests/unit/algorithms/test_grpo.py -k 'dynamo or trtllm or url'                  5 passed
pytest tests/unit/environments/test_nemo_gym.py -k url                                  3 passed

CPU only. I don't have a GPU box, so the vLLM/TRT-LLM tests that need a real engine and the functional suite did not run here. The --trtllm-only selection needed a stub tensorrt_llm locally to get past the conftest gate; the two tests themselves only use _bare_generation and a mocked worker group.

ruff 0.9.9 check, import sort and format are clean on all seven files.

@yupengtang
yupengtang requested review from a team as code owners September 4, 2026 08:45
@copy-pr-bot

copy-pr-bot Bot commented Sep 4, 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.

The four generation backends disagreed on what `dp_openai_server_base_urls`
holds when there is no HTTP server to report. Megatron returned `[]`; vLLM,
TRT-LLM and Dynamo returned a list of `None`s. The placeholder does not stand
for anything -- in those branches no server exists, so the length carries no
information -- but it does make the value lie to a plain truth test, since
`[None]` is truthy. Callers had to know that and write `if not any(urls)`
instead of `if not urls`, and a sync run printed `Reserved 1 vLLM server
URLs: [None]`.

Match Megatron and narrow the type to `list[str]`. `NemoGymConfig.base_urls`
and `spinup_nemo_gym_actor` already declare `list[str]`, so the producers now
match what the consumers ask for; NVIDIA-NeMo#3367 had to add a `cast(list[str], ...)` in
`distillation.py` to bridge the gap.

vLLM needed more than swapping the sentinel. A worker leaves `base_url` unset
unless `expose_http_server` started a server, so an async engine without one
answered `[None] * dp_size` from the workers themselves, not from the guarded
branch. Both `_report_dp_openai_server_base_urls` and `_collect_reserved_urls`
now report that case as empty.

Emptiness is decided for the list as a whole rather than by dropping entries.
`expose_http_server` is one config value for every worker, so the answer is
all-or-nothing; and `GenerationFleetHealth` indexes these URLs by shard and
rejects a list whose length does not match `shard_count`, so filtering entry
by entry would turn a degraded shard into a startup failure with a length
error that names neither cause.

No caller changes needed: `_shard_base_urls` already collapses an all-None
list to None and does the same with an empty one, `_maybe_start_generation_router`
filters falsy entries before its own emptiness check, and NeMo-Gym is reachable
only behind the `should_expose_http_server` assert, so the no-server list never
gets there. Nothing reads the length in the no-server case. The redundant
`any(...)` guards stay as they are.

Signed-off-by: Yupeng Tang <85978465+yupengtang@users.noreply.github.com>
@yupengtang
yupengtang force-pushed the fix/no-server-url-sentinel branch from 333edb3 to 5a13d13 Compare September 5, 2026 10:04
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request waiting-on-maintainers Waiting on maintainers to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generation backends disagree on how to report "no OpenAI server": [None] vs []

2 participants