Skip to content

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

Open
leviking98z-rgb wants to merge 1 commit into
Tencent-Hunyuan:mainfrom
leviking98z-rgb:fix/retire-vllm-patches
Open

leviking98z-rgb wants to merge 1 commit 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.

This branch has not been deployed

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

Labels

need review Ready and waiting for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant