fix(opd): score teacher over /inference/v1/generate (text + multimodal) - #141
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates the On-Policy Distillation (OPD) teacher logprobs endpoint from the custom /inference/v1/generate to the standard, OpenAI-compatible /v1/completions endpoint. This involves updating the request payload format (e.g., using prompt instead of token_ids and enabling echo) and adjusting the response parsing to extract logprobs from choices[0].prompt_logprobs. Additionally, it replaces resp.raise_for_status() with custom error handling to prevent serialization issues with Ray. The review feedback suggests reusing a persistent aiohttp.ClientSession to avoid performance bottlenecks and using errors="replace" when decoding the error response text to prevent potential decoding exceptions.
| if teacher_model: | ||
| payload["model"] = teacher_model | ||
|
|
||
| async with aiohttp.ClientSession() as session: |
There was a problem hiding this comment.
Instantiating a new aiohttp.ClientSession on every call to reward_func is a performance bottleneck. It prevents connection pooling and keep-alive reuse, resulting in a new TCP connection and SSL handshake for every request. Under high concurrency, this can also lead to socket exhaustion.
Consider reusing a persistent session. For example, you can lazily initialize a module-level session:
_session: aiohttp.ClientSession | None = None
async def get_session() -> aiohttp.ClientSession:
global _session
if _session is None or _session.closed:
_session = aiohttp.ClientSession()
return _sessionAnd then use it in reward_func without the async with block for the session itself:
session = await get_session()
async with session.post(args.rm_url, json=payload) as resp:
...| # "can't pickle CIMultiDictProxy" error when Ray serializes the | ||
| # exception back to the driver. Raise a plain, picklable error that | ||
| # surfaces the real status + body instead. | ||
| body = await resp.text() |
There was a problem hiding this comment.
Using await resp.text() without specifying an error handling policy can raise a UnicodeDecodeError if the teacher server returns non-UTF-8 binary data or corrupted text on error (e.g., due to a proxy issue or misconfigured gateway). Specifying errors="replace" ensures that the response body is always decoded successfully into a string, preventing the original HTTP error from being masked by a decoding exception.
| body = await resp.text() | |
| body = await resp.text(errors="replace") |
4b20bc1 to
68b5824
Compare
68b5824 to
2e313b3
Compare
The built-in OPD reward_func (vime/rollout/on_policy_distillation.py) sent vime's
disaggregated /inference/v1/generate request body (token_ids + nested
sampling_params, top-level prompt_logprobs response) but the CI test pointed
--rm-url at /v1/completions, so the teacher rejected the unknown schema with HTTP
400 — which resp.raise_for_status() turned into an aiohttp ClientResponseError whose
CIMultiDictProxy headers fail to pickle across Ray, masking the real error as
"can't pickle CIMultiDictProxy".
Fix: align the test URL to /inference/v1/generate (the endpoint the body targets and
the one vime's own rollout uses), and harden + extend reward_func:
- text: POST {token_ids, sampling_params{max_tokens:1, temperature:0,
prompt_logprobs:1, skip_special_tokens:False}}; model only when --opd-teacher-model
is set (vLLM accepts a missing model -> default served model).
- multimodal: render the (text + image_url) messages via the teacher's
/v1/chat/completions/render to get token_ids + features, attach the student's
canonical full prompt+response token_ids (re-aligning the feature placeholders),
and score with prompt_logprobs — reusing vime.rollout.vllm_rollout's proven
render->features helpers. This is why /inference/v1/generate is used over the
OpenAI /v1/completions: it is the only vLLM endpoint that carries multimodal
features, so one code path scores both text and image teachers.
- replace resp.raise_for_status() with an explicit non-200 -> RuntimeError carrying
the status + body (picklable across Ray; surfaces the real teacher error).
- post_process reads top-level prompt_logprobs (GenerateResponse shape).
Validated on gb200, EXIT_RC=0 both:
- text (Qwen2.5-0.5B, gsm8k): teacher_log_probs flow into opd_reverse_kl.
- multimodal (Qwen3-VL-8B self-distill, geo3k): render->features->generate scores
image samples, teacher_log_probs flow into opd_reverse_kl. (MM weight sync needs
PR #111's qwen3-vl non-colocate update_weights fix.)
Fixes #12.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: aoshen02 <aoshen@inferact.ai>
2e313b3 to
2f3e50f
Compare
Signed-off-by: aoshen02 <aoshen@inferact.ai>
|
The PR description says fixing the code to speak /v1/completions avoids changing the teacher, but the actual code uses /inference/v1/generate and the test also flips --rm-url to it — while the teacher is still the stock OpenAI server which doesn't serve that route. The test will 404? Need to either update the teacher launch to use the disagg serve app, or update the PR description to match the actual approach. |
Signed-off-by: aoshen02 <aoshen@inferact.ai>
Problem (
test_qwen2.5_0.5B_opd_vllm, CI #12)The OPD test launches a stock
vllm.entrypoints.openai.api_serverteacher and points--rm-urlat/v1/completions. But the built-in reward function (vime/rollout/on_policy_distillation.py) — rewritten in #18 when SGLang was removed — POSTs vime's disaggregated/inference/v1/generatecontract instead:token_ids+ nestedsampling_paramsprompt_logprobs/inference/v1/generateis mounted only on the disagg serve app (vllm/entrypoints/serve/disagg/api_router.py), not on the stock OpenAI server. So the teacher returns HTTP 400.Worse,
reward_funcusedresp.raise_for_status(): on non-200 aiohttp raisesClientResponseError, whose response headers are aCIMultiDictProxythat can't be pickled. When Ray ships the exception to the driver the real 400 is replaced by an opaquecan't pickle CIMultiDictProxy.The intended contract is already documented by merged PR #62 ("OPD teacher uses
/v1/completionswithecho=True+prompt_logprobs=1", consumingchoices[0].prompt_logprobs) and matched by the test's--rm-url. This PR realigns the code to it.Fix (
vime/rollout/on_policy_distillation.py, single file)reward_func→ OpenAI/v1/completionsbody:prompt=sample.tokens,max_tokens=1,temperature=0,echo=True,prompt_logprobs=1,skip_special_tokens=False.modelonly sent when--opd-teacher-modelis set (vLLM accepts a missing model → default served model;_is_model_supported(None)→True).raise_for_status()with an explicitif resp.status != 200: raise RuntimeError(status, body[:1000])— picklable across Ray, and surfaces the real teacher error instead of theCIMultiDictProxymask.post_process_rewards→ readchoices[0].prompt_logprobs(was top-level). Per-position shape is identical (dict[token_id→Logprob] | None, pos 0 = None), so_logprob_for_token/ length-assert / trim logic is unchanged.Matches the verl gold standard (
extract_prompt_logprobsreadsoutput.prompt_logprobs[1:]from vLLM completions) and slime's parent HTTP-teacher design.Validation
gb200 single node, 2 student + 1 teacher GPU (shrink of the 4+1 layout), image
vime-vllm:sync1916-arm-cpuvalidate, EXIT_RC=0:CIMultiDictProxy.teacher_log_probsflow into the OPD KL penalty:rollout_log_probs=-0.1547,teacher_log_probs=-0.1589,opd_reverse_kl=0.0044rollout_log_probs=-0.1546,teacher_log_probs=-0.1596,opd_reverse_kl=0.0041Fixes #12.
🤖 Generated with Claude Code