[Clarification] Rename launch_vllm.py -> launch_vllm_hidden_states.py#820
[Clarification] Rename launch_vllm.py -> launch_vllm_hidden_states.py#820WindChimeRan wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesThe PR standardizes hidden-state extraction workflows on Hidden-states launcher migration
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bb8f37d to
88eb70a
Compare
WindChimeRan
changed the title
The script always configures vLLM as a hidden-states producer (extract_hidden_states + a kv_producer kv_transfer_config writing to --hidden-states-path), but its generic name reads like a plain-server launcher. Rename to make the specialization obvious, update all references, and add a one-line module docstring plus a CLI-ref note pointing plain-server users (e.g. response regeneration) at `vllm serve`. Pure rename + reference updates; no behavior change. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
88eb70a to
4a75a77
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/cli/launch_vllm_hidden_states.md`:
- Line 5: Update the documentation sentence near the `--hidden-states-path`
reference to qualify that requests write tensors there only when using the
default file-based `--hidden-states-backend`, or describe the destination
behavior for each supported backend. Ensure the statement does not imply that
non-file backends write to this path.
🪄 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: Pro
Run ID: 1ab09816-05aa-4cd6-9dca-42092f508f7a
📒 Files selected for processing (23)
docs/.nav.ymldocs/cli/data_generation_offline.mddocs/cli/index.mddocs/cli/launch_vllm_hidden_states.mddocs/cli/train.mddocs/user_guide/tutorials/train_dflash_online.mddocs/user_guide/tutorials/train_eagle3_offline.mddocs/user_guide/tutorials/train_eagle3_online.mddocs/user_guide/tutorials/train_mtp_online.mddocs/user_guide/tutorials/train_peagle_offline.mdexamples/train/dflash_qwen3_8b_sharegpt_online_5k.shexamples/train/dspark_qwen3_0_6b_sharegpt_online.shexamples/train/eagle3_llama3_8b_ultrachat_offline_5k.shexamples/train/eagle3_qwen3_8b_sharegpt_online_5k.shexamples/train/mtp_qwen3_5_9b_gsm8k_online.shexamples/train/peagle_qwen3_8b_sharegpt_online_5k.shhs_connectors/src/hs_connectors/transfer.pyscripts/launch_vllm_hidden_states.pytests/e2e/regression/test_eagle3_offline_acceptance.pytests/e2e/regression/test_eagle3_online_acceptance.pytests/e2e/smoke/test_offline_training.pytests/e2e/smoke/test_online_training.pytests/e2e/utils.py
|
|
||
| Launches a vLLM server configured for hidden states extraction, used for online training or offline hidden states generation. | ||
|
|
||
| Every request writes hidden-state tensors to `--hidden-states-path`; for a plain server (e.g. response regeneration) use `vllm serve`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the output-path claim for alternate backends.
The launcher supports selectable --hidden-states-backend implementations, while --hidden-states-path is backend-specific. Saying every request writes tensors to that path is inaccurate for non-file backends. Limit this statement to the default file backend or document backend-specific destinations.
As per path instructions, documentation changes must be accurate and complete.
🤖 Prompt for 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.
In `@docs/cli/launch_vllm_hidden_states.md` at line 5, Update the documentation
sentence near the `--hidden-states-path` reference to qualify that requests
write tensors there only when using the default file-based
`--hidden-states-backend`, or describe the destination behavior for each
supported backend. Ensure the statement does not imply that non-file backends
write to this path.
Source: Path instructions
|
I'm not entirely sold on this. We already have docstrings and an argparse description making it clear this is for hidden states extraction. I get that we could make this more explicit, but I'm not convinced its necessary. |
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews
🔴 Require approval from approved reviewers listWaiting for any of
This rule is failing.All pull requests must have at least one approving review from a member of the approved reviewers list before merging.
|
No one actually read it (both human and agent). issue: people & agent frequently use Also
I recommend ship the file renaming first (file renaming is the smallest fix for it), then re-design the UX, by merging some scripts functionalities to the src/ |
|
This pull request has merge conflicts that must be resolved before it can be |
2 participants
Purpose
TLDR:
scripts/launch_vllm.pyis slow because it always producing hidden states. So the name should not be generic.scripts/launch_vllm.pyalways configures vLLM as a hidden-states producer (writes tensors to--hidden-states-pathon every request), but its generic name reads like a plain-server launcher. So it gets reached for in workflows that only wantvllm serve(e.g. response regeneration).Rename it to
launch_vllm_hidden_states.py, update all references, and add a one-line module docstring plus a one-sentence CLI-ref note. Pure rename + reference updates; no behavior change.Tests
git grep 'launch_vllm\.py'/launch_vllm.md/launch_vllm/→ all empty.Checklist
I have filled in: