Skip to content

fix(policy): surface the uncovered-rank error during HTTP weight refit - #3515

Closed
tianyi-zhang-02 wants to merge 6 commits into
NVIDIA-NeMo:mainfrom
tianyi-zhang-02:fix/http-refit-uncovered-rank
Closed

tianyi-zhang-02 wants to merge 6 commits into
NVIDIA-NeMo:mainfrom
tianyi-zhang-02:fix/http-refit-uncovered-rank

Conversation

@tianyi-zhang-02

@tianyi-zhang-02 tianyi-zhang-02 commented Aug 6, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes the existing uncovered-rank error in stream_weights_via_http_impl reachable.

The function currently reads cached IPC topology before resolving the rollout-engine URL, so an uncovered rank raises KeyError: 'gather_src'. This moves those reads below URL resolution, allowing the intended RuntimeError to report the bad rank mapping.

Covered-rank behavior is unchanged.

Testing

  • Added uncovered-rank and covered-rank regression tests: 2 passed locally
  • ruff check, ruff format, and pyrefly pass
  • Three existing ZMQ IPC tests fail identically on main and this branch on Windows because ipc:// is unsupported

Issues

Found while investigating #3288. Draft PRs #3330 and #3426 also touch this function.

cc @RayenTian @yuki-97

stream_weights_via_http_impl read gather_src/gather_group out of worker_state
before resolving the engine URL. _ensure_ipc_topology only stashes those keys
for ranks inside a matched range, so a rank no engine covers died with a bare
KeyError('gather_src') and never reached the descriptive RuntimeError written
for exactly that case.

Move the worker_state reads below the engine resolution block. The covered
path is unchanged: the resolution loop reads only rollout_engine_urls,
num_gpus_per_engine and rank, and _ensure_ipc_topology still runs first so
every rank enters the new_group collective.

Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
@tianyi-zhang-02
tianyi-zhang-02 requested review from a team as code owners August 6, 2026 07:22
@copy-pr-bot

copy-pr-bot Bot commented Aug 6, 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.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Aug 8, 2026

@yuki-97 yuki-97 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.

@bg51717 to review

@yuki-97
yuki-97 requested a review from bg51717 August 17, 2026 03:12
@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-maintainers Waiting on maintainers to respond label Aug 17, 2026
…ainer-update

Signed-off-by: bg51717 <biguo@nvidia.com>
Signed-off-by: bg51717 <biguo@nvidia.com>

@bg51717 bg51717 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. This change ensures uncovered policy ranks raise the intended RuntimeError instead of a KeyError, with tests covering both covered and uncovered rank paths.

@bg51717

bg51717 commented Aug 18, 2026

Copy link
Copy Markdown
Member

Merged the latest upstream/main into this branch and removed the redundant explanatory comment.

@bg51717 bg51717 added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Aug 18, 2026
@bg51717

bg51717 commented Aug 18, 2026

Copy link
Copy Markdown
Member

/ok to test 78f147d

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-customer Waiting on the original author to respond label Aug 18, 2026
@tianyi-zhang-02

tianyi-zhang-02 commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Two things on the red checks.

L0_Unit_Tests_Sglang was flaky, not a real failure. That run ended with

=========== 79 passed, 10 skipped, 61 warnings in 1026.58s (0:17:06) ===========
./tests/run_unit.sh: line 44: 20394 Segmentation fault (core dumped) pytest $pytest_args

All 79 passed. The segfault is at interpreter shutdown, after the summary line. The job is judged by tail -n 1 err.log | grep "Finished successfully.", and that never gets written when the process dies on a signal, so the whole shard goes red. CI quality check is just the roll-up of it. Nothing here runs at shutdown — the PR moves two reads inside stream_weights_via_http_impl.

I also brought the branch up to date. Should be clean merge now.

Needs a fresh /ok to test when convenient :)

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-customer Waiting on the original author to respond labels Aug 21, 2026
@tianyi-zhang-02

tianyi-zhang-02 commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Closing this — main fixed it, and better than I did.

stream_weights_via_http_impl was rewritten since I opened this. It now reads the topology with .get() instead of [...], and an uncovered rank takes an explicit placeholder branch that drains the bucket iterator and returns. That's the right answer: the iterator lazily drives export_hf_weights, whose PP broadcast and TP gather are collective over the whole trainer world, so raising there — which is what this PR did — would have hung the other ranks.

Thanks for the review @bg51717 and @yuki-97, sorry it went stale. :)

@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-maintainers Waiting on maintainers to respond label Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) community-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants