Skip to content

fix(vllm-omni): retire per-request AR seed patch - #474

Merged
CjhHa1 merged 2 commits into
Tencent-Hunyuan:mainfrom
leviking98z-rgb:fix/retire-vllm-patches
Sep 24, 2026
Merged

CjhHa1 merged 2 commits into
Tencent-Hunyuan:mainfrom
leviking98z-rgb:fix/retire-vllm-patches

Conversation

@leviking98z-rgb

Copy link
Copy Markdown
Collaborator

Summary

Remove the vLLM-Omni per-request AR seed monkey patch and its patch-table entry.

The patch replaced every unset SamplingParams.seed with os.urandom, based on
the assumption that reusing one SamplingParams object collapses a GRPO group to
identical tokens. vLLM 0.27 and 0.28 instead keep seed=None on the ordinary
random-sampling path, where the GPU RNG advances across batch rows. The override
is therefore unnecessary for UniRL's current single-replica recipes and makes
rollouts impossible to reproduce from the configured engine seed.

Related Issue

Refs #473.

Test Plan

  • SKIP=no-commit-to-branch pre-commit run --all-files --show-diff-on-failure — passed.
  • python -m compileall -q unirl/rollout/engine/vllm_omni/patches — passed.
  • One-off sampler probe with vllm==0.28.0, vllm-omni==0.28.0,
    torch==2.13.0+cu130 on one NVIDIA H20: 64 seed=None rows produced 55
    distinct tokens, and resetting the Torch/CUDA seed reproduced the complete
    batch. No model checkpoint or dataset is involved in this sampler-level check.
  • Source-checked vLLM v0.27.0: seed=None likewise avoids a per-request
    generator and uses the advancing batched GPU RNG.

Compatibility / Risk

Current UniRL vLLM-Omni recipes use one engine replica, so request diversity is
preserved by vLLM's advancing RNG. Independently initialized multi-replica
engines can still begin with correlated RNG streams; if that topology is added,
it should use deterministic seeds derived from request identity rather than host
entropy from os.urandom.

No config, checkpoint, data-format, or API migration is required.

Reviewer Notes

Open PR #413 removes the separate capture-flush direct registration as part of
the vLLM-Omni 0.28 migration, but retains this seed patch. No open PR was found
that removes or replaces the seed override.

AI assistance was used. The full two-file diff and all reported validation
output were reviewed before publication.

Checklist

  • I reviewed the changed code and removed unrelated/generated artifacts.
  • I updated tests, docs, and configs where needed, or explained why not.

@CjhHa1 CjhHa1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions Bot added approved Approved by reviewer and removed need review Ready and waiting for review labels Sep 24, 2026
@CjhHa1
CjhHa1 merged commit 67d3339 into Tencent-Hunyuan:main Sep 24, 2026
6 checks passed
@github-actions github-actions Bot removed the approved Approved by reviewer label Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants