[Data] Derive off-policy loss masks from vLLM render boundaries#794
[Data] Derive off-policy loss masks from vLLM render boundaries#794WindChimeRan wants to merge 19 commits into
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe preprocessing pipeline now uses a vLLM render endpoint to derive assistant loss boundaries, adds pre-tokenized passthrough handling, updates the CLI and public APIs, introduces an HTTP client, and replaces legacy assistant-pattern tests with render-boundary coverage. Render-Based Dataset Preprocessing
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
e5f7fb8 to
5a36d39
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/prepare_data.md`:
- Line 45: Update both off-policy conversation command examples in the
documentation to include the required --render-endpoint argument, using the same
vLLM server URL format described in the option reference. Apply the addition
consistently to the full example so all commands match the current CLI API.
In `@src/speculators/data_generation/preprocessing.py`:
- Around line 246-249: Update the preprocessing loop around the prompt_ids
length check to continue scanning after an over-length prompt instead of
breaking, so later history-rewritten contexts can still produce supervised
assistant rows when they fit within max_length. Add a regression test covering
an over-length turn followed by a shorter rewritten prompt and verify the later
turn is retained.
- Around line 415-418: Update the multimodal preprocessing path around
results["messages"] to preserve each row’s tools alongside messages. Extend the
data passed through vllm_client.ClientItem so the render path can send those
tools back when generating input_ids, preserving identical prompt-token
round-tripping for tool-use examples.
In `@src/speculators/data_generation/render_client.py`:
- Around line 50-56: Update the response classification around
InvalidResponseError so transient HTTP 429 responses remain eligible for
`@with_retries`, while deterministic client errors continue to short-circuit
retries. Exclude HTTPStatus.TOO_MANY_REQUESTS from the 4xx condition and
preserve the existing InvalidResponseError behavior for other covered statuses.
- Around line 62-67: Update the response validation around resp.json() and
token_ids to require a dictionary payload containing token_ids as a flat list of
plain non-negative integers; reject nested lists, floats, booleans, negatives,
and other invalid values with RenderError before returning. Preserve the
existing truncated-response context in the validation error and return only the
validated token_ids list.
🪄 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: b76f188b-c076-4959-845f-0db6b9380e9d
📒 Files selected for processing (9)
docs/cli/prepare_data.mdpyproject.tomlscripts/prepare_data.pysrc/speculators/data_generation/preprocessing.pysrc/speculators/data_generation/render_client.pytests/integration/datagen/test_preprocessing.pytests/integration/datagen/test_regex_patterns.pytests/integration/datagen/test_render_boundary.pytests/unit/scripts/test_response_regeneration.py
💤 Files with no reviewable changes (2)
- tests/integration/datagen/test_regex_patterns.py
- tests/integration/datagen/test_preprocessing.py
d29cad4 to
7ed84a4
Compare
Both examples pass `--data sharegpt`, which is off-policy, so as written they
now fail immediately:
ValueError: render_endpoint is required to derive loss masks for
off-policy conversations. Pass --render-endpoint pointing at a vLLM server.
The flag was documented in the argument reference but never reached the
copy-pasteable commands. Note the requirement in Basic Usage as well, since
it is conditional -- pre-tokenized input still needs no endpoint.
Reported by CodeRabbit on vllm-project#794.
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
408 and 429 sit inside the 4xx range, so they were classified as deterministic client errors and raised InvalidResponseError, which short-circuits @with_retries. A proxy shedding load therefore dropped the conversation permanently on the first 429 instead of backing off and retrying. Exclude them from the deterministic branch so they fall through to RenderError, which is retry-eligible. Everything else in 4xx keeps short-circuiting -- retrying a malformed request or a wrong URL wastes requests without changing the outcome. vLLM itself does not emit either status, but --render-endpoint accepts any URL and real deployments put proxies and load balancers in front of it. Tests: parametrized over 408/429, asserting the call is attempted max_retries+1 times. Both fail against the previous classification. Reported by CodeRabbit on vllm-project#794. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
408 and 429 fall inside 4xx, so they raised InvalidResponseError, which short-circuits @with_retries: a proxy shedding load dropped the conversation on the first 429 instead of backing off. Carve them out so they reach the retry-eligible RenderError; the rest of 4xx still short-circuits. vLLM emits neither, but --render-endpoint takes any URL and deployments front it with proxies that do. Reported by CodeRabbit on vllm-project#794. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
19bcfc7 to
ab44652
Compare
speculatorsbot
left a comment
There was a problem hiding this comment.
LGTM. Clean replacement of the two-tier local masking path (HF template probe + regex span detection) with a single render-boundary mechanism. The boundary derivation is correct, the LCP fallback for scaffold-divergent templates (DeepSeek-R1 / Qwen3.5) is load-bearing and well-validated, and the monotonicity argument for the early-exit break holds. Net deletion of ~1k lines. Agree with deferring the multimodal tools round-trip and the async/batched client to follow-ups. Recommend approving.
🤖 Generated with Claude Code using the /pr-review skill
|
This pull request has merge conflicts that must be resolved before it can be |
Both examples pass `--data sharegpt`, which is off-policy, so as written they
now fail immediately:
ValueError: render_endpoint is required to derive loss masks for
off-policy conversations. Pass --render-endpoint pointing at a vLLM server.
The flag was documented in the argument reference but never reached the
copy-pasteable commands. Note the requirement in Basic Usage as well, since
it is conditional -- pre-tokenized input still needs no endpoint.
Reported by CodeRabbit on vllm-project#794.
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
408 and 429 fall inside 4xx, so they raised InvalidResponseError, which short-circuits @with_retries: a proxy shedding load dropped the conversation on the first 429 instead of backing off. Carve them out so they reach the retry-eligible RenderError; the rest of 4xx still short-circuits. vLLM emits neither, but --render-endpoint takes any URL and deployments front it with proxies that do. Reported by CodeRabbit on vllm-project#794. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
ab44652 to
c0396ee
Compare
Replace the two-tier local masking path (HF {% generation %} assistant-mask
probe + regex span detector) with a single mechanism: render each conversation
through the training vLLM instance's /v1/chat/completions/render endpoint and
take the loss mask from the boundary between the prompt render and the full
render of each assistant turn. Append-only templates pack to one row per
conversation; history-rewriting (reasoning) templates fan out to one row per
turn. On-policy pre-tokenized rows (vllm-project#729) still pass straight through.
One tokenizer now produces the mask, the target hidden states, and the serving
prompt. Deletes ~270 lines of regex/tag masking and the local HF render path;
adds render_client.py (token_ids only, no truncation) and boundary derivation.
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
- drop redundant early-return in _render_boundary_rows (subsumed by the no-turns guard) - pass an is_multimodal bool into _preprocess_batch instead of the heavyweight processor, so the map closure no longer pickles it into every worker Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
render_client.py imports httpx; it was only present transitively, so a clean install would fail to resolve it. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Tests: - drop test_render_conversation_returns_token_ids (a one-line passthrough; the 200 path is covered by the missing-token_ids test) - drop test_context_filling_window_yields_no_rows (single-guard edge) - trim test_pretokenized_dataset_skips_render to its load-bearing assertion Prose: - condense the ported _render_boundary_rows docstring and the verbose comments/docstrings in preprocessing.py and render_client.py Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
… vLLM
Drop the packed branch. Every assistant turn now gets its own row carrying
the history re-rendered the way inference sees it, for every template.
Packing was an optimization, never a correctness requirement: it only held
for append-only templates, and thinking models (Qwen3 strips <think> from
history) already fanned out. Removing it deletes _Turn, the append_only
chain check, and the two-pass structure -- one path instead of three.
The cost lands on append-only templates, measured on 10 real multi-turn
ShareGPT conversations rendered by Qwen2.5-0.5B-Instruct:
packed 10 rows, 8606 tokens
fan-out 38 rows, 20449 tokens (3.8x rows, 2.4x tokens)
Qwen3-0.6B output is byte-identical before and after (37 rows), since it
took the fan-out path already.
The <think>-scaffold LCP fallback stays. It is load-bearing, not defensive:
DeepSeek-R1-Distill pre-fills `<think>\n` in the generation prompt, and
Qwen3.5 pre-fills an empty `<think></think>` that recorded reasoning then
contradicts. Both break prefix-stability and land in that branch.
Also warn when --seq-length silently truncates supervision. A row clipped
mid-response keeps thousands of supervised tokens, so it passes the
existing zero-supervision check and looks healthy while its tail and
closing tokens are never learned. On 40 real gsm8k rows with recorded
reasoning at --seq-length 2048, 13 are clipped, discarding a median 34%
of the assistant turn (worst: 1482 of 3530 tokens) -- previously with no
output at all. num_clipped counts only kept-but-truncated rows; rows
clipped past their boundary already report as unsupervised and would
otherwise be counted twice.
Tests:
- add tests/e2e/smoke/test_render_boundary.py -- the first test to exercise
the render transport for real (URL, token_ids key, boundary from what vLLM
actually returns). Qwen3-0.6B, live server, one conversation covering
fan-out, the leading-assistant exclusion, and the trailing-user drop.
- drop the tests that faked the render and are now covered there: the packed
and fan-out routing tests, first-turn-context-only, trailing-dropped, and
test_build_eagle3_dataset_packed_end_to_end (which named itself end-to-end
while pointing at http://fake, and used a template that structurally could
not reach fan-out).
- keep what a live server cannot produce on demand: the scaffold fallback,
the unstable guard, and the client's error paths.
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Both examples pass `--data sharegpt`, which is off-policy, so as written they
now fail immediately:
ValueError: render_endpoint is required to derive loss masks for
off-policy conversations. Pass --render-endpoint pointing at a vLLM server.
The flag was documented in the argument reference but never reached the
copy-pasteable commands. Note the requirement in Basic Usage as well, since
it is conditional -- pre-tokenized input still needs no endpoint.
Reported by CodeRabbit on vllm-project#794.
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
- `assert sum(mask) > 0`: _append_row returns "unsupervised" and does not append when num_valid_tokens == 0, so every row reaching the dataset satisfies this by construction. - `assert len(ids) == len(mask)`: the mask is built as [0]*boundary + [1]*(len(full_ids) - boundary) and both are clipped by the same slice, so they cannot diverge -- and `zip(..., strict=True)` two lines below already raises on skew. - unused `logging` import and module logger. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
408 and 429 fall inside 4xx, so they raised InvalidResponseError, which short-circuits @with_retries: a proxy shedding load dropped the conversation on the first 429 instead of backing off. Carve them out so they reach the retry-eligible RenderError; the rest of 4xx still short-circuits. vLLM emits neither, but --render-endpoint takes any URL and deployments front it with proxies that do. Reported by CodeRabbit on vllm-project#794. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
c0396ee to
63cfcf5
Compare
speculatorsbot
left a comment
There was a problem hiding this comment.
Re-reviewed after the 7 commits since my previous pass (ab44652..63cfcf5). The always-fan-out simplification is a good call -- removing the packed/append-only branch deletes _Turn, the two-pass chain check, and the dual routing logic in exchange for a measured 2.4x token cost on append-only templates, which is acceptable given the complexity reduction. The e2e test against a live Qwen3-0.6B server (test_render_boundary.py) is the most valuable addition: it is the only test that exercises the actual /render transport and catches contract drift, which the stubbed unit tests structurally cannot. The transient-4xx retry fix (TRANSIENT_STATUSES for 408/429) correctly threads through the existing with_retries/InvalidResponseError short-circuit. All coderabbit findings from the first round are either addressed in these commits or explicitly deferred (multimodal tools round-trip). DCO sign-offs present on all 8 commits. LGTM. Recommend approving.
🤖 Generated with Claude Code using the /pr-review skill
orestis-z
left a comment
There was a problem hiding this comment.
That's a great delta -1.1k lines :)
Some nits on render_client.py (feel free to ignore them)
| num_convs_in = 0 | ||
| num_convs_empty = 0 | ||
|
|
||
| for idx, conv in enumerate(conversations): |
There was a problem hiding this comment.
nit: this method seems to be doing quite a lot, and there's a loop inside a loop. Could extract helper method(s) to hide some of the logic.
There was a problem hiding this comment.
Not sure how to do that cleanly.
…ports Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
fc903f3 to
3e0ba1c
Compare
…sation Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
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.
|
…ing_dataset The prepared dataset is not eagle3-specific: prepare_data.py takes no speculator type, and train.py builds eagle3, dflash, dspark, peagle, and mtp from the same rows. Rename the function and drop EAGLE3 from the two docstrings that described its output. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Point --render-endpoint at the launch_vllm.py instance rather than a second server, and flag the URL-form difference: data_generation_offline takes an /v1-suffixed endpoint, while this one is appended to and 404s on that form. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
The argparse help still carried the pre-change wording. Also scope the requirement correctly: --data is repeatable and the check runs per dataset, so a single raw input among pre-tokenized ones still needs the endpoint. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
|
The quality checks have failed. Please run |
shanjiaz
left a comment
There was a problem hiding this comment.
I think the diff makes sense and checking the diff for loss masks is indeed smart. Verified locally. Could you please address Orestis' style concerns? we could merge soon. Would want to make sure this works on multi-modal data tho. Especially the coco dataset we claim to support.
mypy rejects passing None where ProcessorLike is expected. Neither test reads the processor: the missing-endpoint guard raises before it is used, and pre-tokenized rows skip preprocessing. Name that with a cast sentinel rather than an ignore. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Purpose
Derive the off-policy training loss mask from a render boundary, with the vLLM instance as the single tokenization source. For each assistant turn we render the prompt and the prompt-plus-turn via
/v1/chat/completions/render, and supervise the tokens the second render adds past the first.Then we deprecate the fallback hf tokenization & fallback regex paths
This is the same prompt/completion boundary the engine already reports for on-policy regeneration (#729), reconstructed from the chat template. One tokenizer now produces the loss mask, the target hidden states, and the serving prompt — no separate local tokenization that can drift from serving.
Design write-up (worked examples, rationale): https://claude.ai/code/artifact/6cd3bf57-3d20-4a06-b27d-5b08e5fce6b4
What changes
{% generation %}tag-mask probe and the regex span-detector — plus the local render adapters and--assistant-pattern./renderclient and the boundary derivation.Why
token_ids, not the server mask:/rendercan also returnassistant_tokens_mask, but it's only populated when the template carries{% generation %}tags, which the common models don't ship. We request onlytoken_idsand compute the mask from the boundary — so it works on every template, and isn't blocked on the unreleasedreturn_loss_mask(/renderitself shipped in v0.15.0).Requirement
--render-endpointis now required for off-policy data (pre-tokenized input still works without it). Point it at the vLLM instance already used for hidden-state extraction, or a GPU-lessvllm launch render. Single source of truth holds only if that instance'stokenizer_modematches serving's.How to review
~68% of the diff is pure deletion; the real surface is ~440 lines of source:
render_client.py, the boundary functions,test_render_boundary.py_preprocess_batch,build_eagle3_dataset,load_and_preprocess_datasetswitching to the boundary, plus the CLI swapThe other preprocessing helpers are byte-identical to
main. The delete-plus-rewire core is atomic;render_client.pyis the one piece reviewable on its own.Tests
It prints the derived mask inline (blue = trainable, grey = masked):
The boundary is the thing to check: everything from
<think>on is blue, while the user turn and the<|im_start|>assistantheader stay grey.Or automated — launches its own vLLM and asserts the masks:
Deferred / follow-ups
datasets.map(num_proc). A batched/async client if throughput needs it.Checklist