refactor(regen): one dataset registry, every preset usable both on&off-policy #777
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe response regeneration script now consumes shared dataset presets, applies preset filtering and normalization before turn extraction, and uses attribute-based metadata. Dataset prompt fields, tests, and CLI documentation were updated accordingly. Dataset-configured response regeneration
Possibly related issues
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 |
e2bba46 to
9db7928
Compare
4486025 to
b0be6d8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/cli/response_regeneration.md (1)
72-78: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
--subsetin the Data Arguments section The CLI exposes--subsetinscripts/response_regeneration/script.py, but it’s missing from this list between--splitand--limit. Add a short entry for completeness.🤖 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/response_regeneration.md` around lines 72 - 78, Add a `--subset` entry to the Data Arguments section between `--split` and `--limit`, briefly describing its purpose and matching the existing option-format style in the response regeneration documentation.
🧹 Nitpick comments (1)
scripts/response_regeneration/script.py (1)
144-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding type annotations to
prepare_rowfor mypy consistency.Adjacent functions like
_primary_identifierannotaterow: dict[str, Any], whileprepare_rowandextract_turnsare unannotated. Annotatingprepare_rowwould let mypy verifyconfig.filter_fn,config.normalize_fn, andconfig.prompt_fieldattribute access at call sites.As per coding guidelines, verify type annotations are consistent with mypy requirements for
**/*.pyfiles.♻️ Optional: add type annotations
-from speculators.data_generation.configs import DatasetConfig +from typing import Any + +from speculators.data_generation.configs import DatasetConfig -def prepare_row(row, config): +def prepare_row(row: dict[str, Any], config: DatasetConfig) -> list[dict[str, str]]: """Extract regeneration turns from a raw dataset row, ``[]`` to skip it.🤖 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 `@scripts/response_regeneration/script.py` around lines 144 - 154, Annotate prepare_row with the repository’s established row and config types, and add the appropriate return type for its extracted turns or skip sentinel. Reuse existing type aliases or definitions used by _primary_identifier, extract_turns, and the filter_fn/normalize_fn configuration fields so mypy can validate attribute access without introducing duplicate types.
🤖 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.
Outside diff comments:
In `@docs/cli/response_regeneration.md`:
- Around line 72-78: Add a `--subset` entry to the Data Arguments section
between `--split` and `--limit`, briefly describing its purpose and matching the
existing option-format style in the response regeneration documentation.
---
Nitpick comments:
In `@scripts/response_regeneration/script.py`:
- Around line 144-154: Annotate prepare_row with the repository’s established
row and config types, and add the appropriate return type for its extracted
turns or skip sentinel. Reuse existing type aliases or definitions used by
_primary_identifier, extract_turns, and the filter_fn/normalize_fn configuration
fields so mypy can validate attribute access without introducing duplicate
types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60b004a2-b340-413e-b550-0681b7625c9f
📒 Files selected for processing (4)
docs/cli/response_regeneration.mdscripts/response_regeneration/script.pysrc/speculators/data_generation/configs.pytests/unit/scripts/test_response_regeneration.py
orestis-z
left a comment
There was a problem hiding this comment.
LGTM. Clean dedup of the dataset registry per RFC #702 — verified all three original presets produce identical results through the new prepare_row path, resume identity is preserved, and import weight is unchanged (script already imports from speculators). Agree with @coderabbitai's suggestion to document --subset in the Data Arguments section. Recommend approving.
🤖 Generated with Claude Code using the /pr-review skill
orestis-z
left a comment
There was a problem hiding this comment.
LGTM. Both prior items addressed: --subset is documented and extract_turns/prepare_row are annotated. The multimodal gating (MULTIMODAL_DATASETS + _dataset_choice) is sound — the test exhaustively verifies every preset is accounted for. Recommend approving.
🤖 Generated with Claude Code using the /pr-review skill
The response-regeneration script kept its own plain-dict DATASET_CONFIGS (magpie/ultrachat/gsm8k) alongside the DatasetConfig registry in speculators.data_generation.configs, so adding a dataset meant two edits in two formats. Since vllm-project#716 the script imports speculators.data_generation.vllm_client anyway, so consuming the shared registry adds no import weight. DatasetConfig gains an optional prompt_field — the bare-prompt fallback column used when a row carries no conversation — and presets defining it are the script's --dataset choices, which stay exactly magpie/ultrachat/gsm8k with unchanged HF paths, splits, and subsets. Dataset-registry half of RFC vllm-project#702. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
The regen script previously accepted only presets with a bare-prompt column (magpie/ultrachat/gsm8k) — a historical artifact of its Magpie-specific origin, not an architectural limit. Extraction now mirrors the off-policy ingestion via prepare_row(): filter_fn sees the raw row, normalize_fn is merged over it with HF map semantics, and system/user turns are extracted from the result. Every DATASET_CONFIGS preset is therefore a valid --dataset choice, and any dataset that works off-policy works on-policy by construction (nemotron's input/output schema, sharegpt's native conversations, ...). Resume identity is untouched: _primary_identifier still hashes the raw row, so --resume stays valid for outputs from earlier runs. For the three previous presets the extracted turns are unchanged (gsm8k's normalizer yields the same single user turn its prompt_field fallback did); prompt_field remains as a per-row fallback for rows whose conversation is unusable. Known limits deferred to follow-ups rather than gated: sharegpt4v_coco needs an image-capable endpoint and local COCO files (same requirement and failure mode as off-policy), and tool-use datasets get plain-text regeneration until tool-call regen lands. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
The CLI doc's hand-copied dataset list had already drifted (gsm8k was never added); point at the shared registry instead of maintaining a third copy, and document the prepare-data-parity ingestion. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
sharegpt4v_coco normalizes to HF chat-template image parts
({"type": "image", "path": ...}), which the OpenAI Chat Completions API
does not accept: vLLM rejects them with HTTP 400 "Unsupported chat content
part type: 'image'". _post_chat treats 400 as permanent, so every row would
land in the error file.
Emitting image_url parts instead would clear the 400 but not the underlying
gap: the pre-tokenized output row carries input_ids/loss_mask and no pixel
data, so the image positions would train against placeholder-token hidden
states. Gate the preset out of --dataset with a reason and leave on-policy
multimodal to a follow-up.
Also annotate extract_turns/prepare_row and document --subset.
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
68d577c to
12f02aa
Compare
The assertion could not fail: REGEN_DATASETS is derived as DATASET_CONFIGS - MULTIMODAL_DATASETS, so the union identity held for any registry contents, including a new multimodal preset left off the gate list. The rejection behaviour stays covered by test_dataset_choice_rejects_multimodal. Also state why the gate lives in the script: it encodes a current regen limitation, not a dataset property (off-policy dispatches on content-part type). Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
1b08052 to
de1e7dc
Compare
Register nvidia/When2Call (train_sft config) in the shared dataset registry. Since vllm-project#777 that registry is the single source of presets, so the entry serves both pipelines: off-policy `prepare-data` and the on-policy regeneration script's `--dataset`. When2Call is a multi-turn function-calling SFT dataset (CC-BY-4.0) that also teaches when NOT to call a tool. Its conversations use the OpenAI `messages` schema (normalized to `conversations`), and its tool schemas ride along in a parallel `tools` column. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRRGNt2T9ktWyFRboNUZNU Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Register NousResearch/hermes-function-calling-v1 (func_calling config) in the shared dataset registry. Since vllm-project#777 that registry is the single source of presets, so the entry serves both pipelines: off-policy `prepare-data` and the on-policy regeneration script's `--dataset`. Hermes func_calling is a multi-turn function-calling SFT dataset (Apache-2.0) in ShareGPT from/value form, so it needs no normalizer (like the existing sharegpt preset). Tool calls/results are text-encoded (<tool_call>/ <tool_response>) inline; the parallel `tools` column carries the JSON schemas as a JSON string. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRRGNt2T9ktWyFRboNUZNU Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Register nvidia/When2Call (train_sft config) in the shared dataset registry. Since vllm-project#777 that registry is the single source of presets, so the entry serves both pipelines: off-policy `prepare-data` and the on-policy regeneration script's `--dataset`. When2Call is a function-calling SFT dataset (CC-BY-4.0) that also teaches when NOT to call a tool. Every row is a single user/assistant exchange with no system turn; the conversation uses the OpenAI `messages` schema (normalized to `conversations`), and the tool schemas ride along in a parallel `tools` column. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRRGNt2T9ktWyFRboNUZNU Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Register NousResearch/hermes-function-calling-v1 (func_calling config) in the shared dataset registry. Since vllm-project#777 that registry is the single source of presets, so the entry serves both pipelines: off-policy `prepare-data` and the on-policy regeneration script's `--dataset`. Hermes func_calling is a function-calling SFT dataset (Apache-2.0) already in ShareGPT from/value form, so it needs no normalizer (like the existing sharegpt preset). Each row is a single user request followed by a multi-step agentic trajectory: system -> human -> gpt -> tool -> gpt in the common case. Tool calls and results are text-encoded (<tool_call>/<tool_response>) inline; the parallel `tools` column carries the JSON schemas as a JSON string. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRRGNt2T9ktWyFRboNUZNU Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
## Purpose Add the **Hermes function-calling** dataset (`NousResearch/hermes-function-calling-v1`, `func_calling` config) as a predefined preset — a function-calling SFT set (Apache-2.0) originally for post-training. why: - Tool-use training fixture for the tool-call speculator work, alongside When2Call (#769). - Hermes `func_calling` is the canonical function-calling SFT dataset (used to train Hermes 2 Pro) — genuine tool-call → tool-result → answer trajectories. - License **Apache-2.0**. Verified **no overlap** with the `nvidia/SPEED-Bench` eval set (SPEED-Bench contains no function-calling data; Hermes is not among its 24 sources). Rebased onto `main` after #777, the followup that collapsed the two dataset registries into one. This is now a **single** entry in `src/speculators/data_generation/configs.py` — since #777 the regeneration script consumes that same registry, so the preset reaches both pipelines (`prepare-data` and `--dataset`) by construction, with no change to `scripts/response_regeneration/script.py`. `docs/cli/response_regeneration.md` gains the matching row. ### Notes for reviewers - The dataset is already in `conversations` / ShareGPT `from,value` form (same shape as the existing `sharegpt` preset), so it needs **no `normalize_fn`** — the offline `_normalize_conversation` maps `from/value` → canonical roles natively (`system → user → assistant → tool → assistant`, verified). - **Shape**, measured over 1299 rows parsed from `func-calling.json`: each row is a *single* user request followed by a multi-step agentic trajectory. Raw `from` sequences: `system → human → gpt → tool → gpt` (927), `system → human → gpt` (199), `system → human → gpt → tool` (163), `system → human → tool → gpt` (10). 1100/1299 rows carry a tool-result turn; **no** row has more than one `human` turn. "Multi-turn" here means the agentic exchange, not multiple user turns. - Tool calls/results are **text-encoded** (`<tool_call>` / `<tool_response>`) inside the turn value, not structured `tool_calls` fields. Offline training on this is self-consistent (the system turn embeds `<tools>`; the assistant emits `<tool_call>` text). The structured tool-aware path (#750) will additionally need `<tool_call>` parsing, and must avoid double-injecting the tool schema (the system turn already contains `<tools>`) — out of scope here. - The `tools` column is a single JSON string, which `_parse_conv_tools` consumes via `json.loads`. - **On-policy note.** #777 makes every registry preset a valid `--dataset`, so this entry also enables `--dataset hermes-fc`. That is coherent here: `prepare_row()` yields `[system, user]` and the system turn still carries the `<tools>` schemas, so the model *does* see the tools and can emit a `<tool_call>`. The tool-result rounds are dropped, so only the first assistant turn is regenerated; full tool-call regen semantics are #750. This is the difference from #769, which is held as draft: When2Call keeps its tool schemas *outside* the conversation (a parallel `tools` column), so on-policy regen there would see no tools at all. ## Tests - `make quality` (ruff check, ruff format, mdformat, mypy): clean. - `pytest tests/unit/scripts tests/unit/data_generation`: **49 passed**. - Schema verified against the real data file — 1299 rows parsed from `func-calling.json`: `id` / `conversations` (from/value) / `tools` (JSON string) / `category` / `subcategory` / `task`, with `<tool_call>` and `<tool_response>` markup present inline. Config/split resolve confirmed against the Hub API (`func_calling` config → `train` split, Apache-2.0). - Registry sanity: `hermes-fc` entry is bare (`normalize_fn is None`); the offline `_normalize_conversation` maps a real Hermes tool conversation to `system/user/assistant/tool/assistant` with the `<tool_call>` text preserved; `prepare_row()` on a real row yields `[system, user]` with the `<tools>` schemas intact; `hermes-fc` appears in the `--dataset` CLI choices with `script.py` byte-identical to `main`. ## Checklist I have filled in: - [x] The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)". - [x] The test plan/results, such as providing test command and pasting the results. - [x] (Optional) The necessary documentation update. - [x] I (a human) have written or reviewed the code in this pr to the best of my ability. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: shanjiaz <zsjwpianpian@gmail.com>
Purpose
Dataset-registry half of RFC #702. The regen script kept its own 3-entry dataset dict alongside the shared registry in
data_generation/configs.py, so adding a dataset means editing both files in two formats (e.g. #769, #771). Unblocked since #716: the script already imports the package.DatasetConfiggainsprompt_field, the bare-prompt fallback column. No behavior change.prepare_row()applies a preset'sfilter_fn/normalize_fnthe same way prepare-data does, so every registry preset is a valid--dataset— datasets work in both pipelines by construction.--resumeidentity and the original three presets' extracted turns are unchanged. Multimodal payloads (sharegpt4v_coco) and tool-call regen semantics are follow-ups.Tests
pytest tests/unit/scripts tests/unit/data_generation: 48 passed (new: nemotron normalize-parity, filter_fn, map-merge fallback)ruff check,ruff format --check,mypy --check-untyped-defs: clean--helpoffers all presets;--dataset sharegptflows through endpoint auto-detectionChecklist
I have filled in: