Repository navigation
[Rust][Benchmark] Support HF ShareGPT datasets in multi-turn mode - #51104
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9144d55c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| crate::datasets::hf_dataset::download_hf_dataset( | ||
| dataset_id, | ||
| config.hf_subset.as_deref(), | ||
| config.hf_split.as_deref(), | ||
| config.num_prompts, | ||
| ) |
There was a problem hiding this comment.
Make HF cache size-aware for multi-turn runs
When a user does a small HF multi-turn run first, such as --num-prompts 1 --dry-run, this call asks download_hf_dataset to cache only that many rows; the helper's cache key is just dataset/config/split and it returns any existing file before checking the requested row count. A later benchmark of the same HF ShareGPT dataset with more conversations will therefore load the undersized cache and load_sharegpt_multi_turn will oversample the few cached conversations, producing repeated conversations and misleading benchmark results. Please either make the HF cache size-aware or ensure this path fetches enough rows when the existing cache is too small.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for catching this. This appears to be a pre-existing issue in the HF dataset cache rather than something introduced by this PR: the current single-turn HF path on main already calls download_hf_dataset(..., config.num_prompts) and relies on the same dataset/config/split-only cache key before oversampling the loaded rows. This PR reuses that existing downloader contract for multi-turn loading without changing its cache behavior.
I agree that making the cache size-aware would improve both single-turn and multi-turn HF benchmarks, but that requires changing the shared cache contract and deserves its own focused fix and regression coverage. I would prefer to keep this PR scoped to enabling ShareGPT-format HF datasets in the existing multi-turn pipeline so the behavior change remains small and reviewable.
|
✅ @xiaguan, CI is now available for this PR.
|
|
Hi @xiaguan, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
|
The failing pre-commit check comes from existing repository-wide ShellCheck findings in unrelated shell scripts; this PR does not modify those files. |
Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: xiaguan <751080330@qq.com>
b9144d5 to
c5f6f87
Compare
|
Hi @xiaguan, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
|
/ci retry |
|
✅ No earlier CI build exists for this PR. Use |
|
/ci run |
|
✅ Triggered Buildkite CI #88720 for commit |
…lm-project#51104) Signed-off-by: xiaguan <751080330@qq.com> Co-authored-by: OpenAI Codex <codex@openai.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…lm-project#51104) Signed-off-by: xiaguan <751080330@qq.com> Co-authored-by: OpenAI Codex <codex@openai.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
vllm bench serveto load ShareGPT-format Hugging Face datasets in multi-turn mode--hf-output-lenand preserve conversation-style--multi-turn-max-turnssemanticsExample
Inferact/codex_swebenchpro_tracesexposes 610 ShareGPT-format conversations through the Hugging Face dataset server. A network dry run loaded one conversation with three turns and 44,103 user-message tokens.Scope
This supports HF configs whose rows use ShareGPT
conversationsentries with alternatingfrom/valuemessages. It intentionally does not add canonical request-trace replay.Duplicate-work check
I searched open PRs for
hf multi-turn benchmark,ShareGPT HuggingFace multi-turn, andInferact/codex_swebenchpro_traces; no PR implementing this support was found.Tests
cargo fmt --all -- --checkcargo nextest run -p vllm-bench— 149 passed, 10 skippedcargo clippy -p vllm-bench --all-targets -- -D warningspre-commit run --files rust/src/bench/README.md rust/src/bench/src/config.rs rust/src/bench/src/multi_turn.rsInferact/codex_swebenchpro_traces— 1 conversation, 3 turns, 44,103 user-message tokensModel evaluation is not applicable because this changes only benchmark dataset loading and does not modify model execution or outputs.
AI assistance was used to inspect the repository, implement the change, and run tests. The human submitter must review and understand every changed line before marking this PR ready.