Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rust/src/bench/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,7 @@ With `--multi-turn`, `--num-prompts` controls the number of **conversations**, n

- `--dataset-name random` — synthetic conversations with controllable per-turn token lengths. Auto-sets `min_tokens` to enforce output length without `ignore_eos`.
- `--dataset-name sharegpt` — loads all turns (not just the first two); filters for entries with ≥ 2 real turns.
- `--dataset-name hf` — downloads a ShareGPT-format HuggingFace config, then loads it like `sharegpt`.

**Prefix sharing** (random dataset): when `--multi-turn-prefix-global-ratio` or `--multi-turn-prefix-conversation-ratio` is > 0, each turn sends a fixed-length message (no history accumulation) composed of a global prefix + per-conversation prefix + unique suffix. The two ratios must sum to < 1.0.

Expand Down
38 changes: 32 additions & 6 deletions rust/src/bench/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -548,13 +548,15 @@ impl BenchConfig {
));
}

// Normalize and validate min/max turns. ShareGPT only consumes max_turns
// (the loader walks all available turns up to the cap), so the
// min/num/max coupling used for synthetic generation does not apply.
if args.dataset_name == DatasetName::ShareGpt {
// Normalize and validate min/max turns. Conversation datasets only
// consume max_turns (the loader walks all available turns up to the
// cap), so the min/num/max coupling used for synthetic generation
// does not apply.
if matches!(args.dataset_name, DatasetName::ShareGpt | DatasetName::Hf) {
if args.multi_turn_max_turns == 1 {
return Err(BenchError::Config(
"--multi-turn-max-turns must be at least 2 for ShareGPT multi-turn".into(),
"--multi-turn-max-turns must be at least 2 for ShareGPT-format multi-turn"
.into(),
));
}
} else {
Expand Down Expand Up @@ -712,7 +714,7 @@ impl BenchConfig {
multi_turn_min_turns,
multi_turn_max_turns,
sharegpt_multi_turn_max_turns: if args.multi_turn
&& args.dataset_name == DatasetName::ShareGpt
&& matches!(args.dataset_name, DatasetName::ShareGpt | DatasetName::Hf)
&& args.multi_turn_max_turns != 0
{
Some(args.multi_turn_max_turns)
Expand Down Expand Up @@ -1040,6 +1042,30 @@ mod tests {
assert_eq!(config.sharegpt_multi_turn_max_turns, Some(20));
}

#[test]
fn test_hf_multi_turn_uses_conversation_turn_cap() {
let args = vec![
"vllm-bench",
"--backend",
"openai-chat",
"--multi-turn",
"--model",
"test-model",
"--dataset-name",
"hf",
"--dataset-path",
"org/sharegpt-dataset",
"--hf-subset",
"sharegpt",
"--multi-turn-max-turns",
"2",
];
let args = parse_args(args);
let config = BenchConfig::from_args(&args).unwrap();

assert_eq!(config.sharegpt_multi_turn_max_turns, Some(2));
}

#[test]
fn test_sweep_summary_percentiles_default_empty() {
let args = base_multi_turn_args();
Expand Down
26 changes: 23 additions & 3 deletions rust/src/bench/src/multi_turn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,29 @@ pub async fn run_multi_turn_benchmark(config: &BenchConfig) -> Result<serde_json
));
}
DatasetName::Hf => {
return Err(BenchError::Config(
"HF dataset multi-turn is not yet supported. Use 'random' or 'sharegpt' with --multi-turn.".into(),
));
let tok = tokenizer
.as_ref()
.ok_or_else(|| BenchError::Config("HF dataset requires a tokenizer".into()))?;
let dataset_id = config.dataset_path.as_deref().ok_or_else(|| {
BenchError::Config("--dataset-path is required for --dataset-name hf".into())
})?;
let (downloaded_path, _config, _split) =
crate::datasets::hf_dataset::download_hf_dataset(
dataset_id,
config.hf_subset.as_deref(),
config.hf_split.as_deref(),
config.num_prompts,
)
Comment on lines +185 to +190

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

.await?;
crate::datasets::multi_turn::load_sharegpt_multi_turn(
tok,
&downloaded_path,
config.num_prompts,
config.hf_output_len,
config.sharegpt_multi_turn_max_turns,
config.seed,
&config.request_id_prefix,
)?
}
};

Expand Down