Repository navigation
Retain v20cb evaluation records #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| { | ||
| "arm": "copilot_v20cb_train", | ||
| "engine": "infermatrix-copilot CLI (shipped pipeline, pr-review@6 adaptive-depth ensemble; issue-answer dry-run)", | ||
| "llm": "per .env (DeepSeek-routed)", | ||
| "dataset": "vllm_omni_dataset.yaml", | ||
| "splits": [ | ||
| "train" | ||
| ], | ||
| "kinds": [ | ||
| "pr_review" | ||
| ], | ||
| "n_items": 10, | ||
| "trace": { | ||
| "AGENT_TRACE": "(default)", | ||
| "AGENT_TRACE_IO": "(default)", | ||
| "AGENT_TRACE_IO_FULL": "(default)" | ||
| }, | ||
| "moa_when": "off", | ||
| "pr_context_mode": "no_discussion", | ||
| "resolved": { | ||
| "moa_when": "off", | ||
| "moa_eligible_full_depth_pr": false, | ||
| "llm_mixture_members": [ | ||
| "mimo-v2.5", | ||
| "qwen3.6-plus" | ||
| ], | ||
| "strict_backend": "cursor", | ||
| "strict_backend_model": "composer-2.5", | ||
| "review_lens_backends": {}, | ||
| "agent_model": "deepseek-v4-pro[1m]", | ||
| "review_planner_model": "deepseek-v4-flash", | ||
| "review_promotion_model": "deepseek-v4-flash" | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| { | ||
| "wall_s": 1003.1, | ||
| "split": "train", | ||
| "rc": 0, | ||
| "run_dir": "/data/zhoutaichang/copilot/InferMatrixCopilot/eval/dataset/arms/copilot_v20cb_train/runs/pr4804/run-20260817-194336-a281f3", | ||
| "recorded_at": "2026-08-17T20:00:17", | ||
| "attempts": [ | ||
| { | ||
| "invocation_id": "inv-c3405671", | ||
| "outcome": "run:run-20260817-194336-a281f3", | ||
| "rc": 0, | ||
| "pre_run": null | ||
| }, | ||
| { | ||
| "run_dir": "run-20260817-194336-a281f3", | ||
| "llm_calls": 0, | ||
| "input_tokens": 0, | ||
| "output_tokens": 0, | ||
| "usd": 0.0, | ||
| "source": "no-trace" | ||
| } | ||
| ], | ||
| "attempt_usd_total": 0.0, | ||
| "llm_calls": 0, | ||
| "input_tokens": 0, | ||
| "output_tokens": 0, | ||
| "usd": 0.0, | ||
| "source": "no-trace", | ||
| "metrics": { | ||
| "schema": 1, | ||
| "run": "run-20260817-194336-a281f3", | ||
| "task_kind": "pr_review", | ||
| "status": "done", | ||
| "quality": { | ||
| "q": null, | ||
| "weight_coverage": 0.0, | ||
| "partial": true, | ||
| "abstained": false, | ||
| "components": { | ||
| "recall_w": null, | ||
| "precision_v": null, | ||
| "useful": null, | ||
| "calib": null, | ||
| "decision": null | ||
| } | ||
| }, | ||
| "cost": { | ||
| "usd": 0.5684, | ||
| "minutes": 16.69, | ||
| "ci_minutes": 0, | ||
| "input_tokens": 1690401, | ||
| "output_tokens": 101793, | ||
| "tool_calls": 466, | ||
| "usd_ref": 1.0, | ||
| "min_ref": 10.0, | ||
| "cost_index": 1.7052, | ||
| "cost_partial": false, | ||
| "unpriced_calls": 0, | ||
| "source": "events", | ||
| "llm_calls": null, | ||
| "by_role": null, | ||
| "cache_read_tokens": null, | ||
| "cache_creation_tokens": null | ||
| }, | ||
| "timings": { | ||
| "steps": { | ||
| "fetch": 4.39, | ||
| "gate": 1.64, | ||
| "review": 995.42, | ||
| "report": 0.06 | ||
| }, | ||
| "time_to_first_result_s": 1001.5 | ||
| }, | ||
| "risk": { | ||
| "incidents": { | ||
| "catastrophic": 0, | ||
| "severe": 0, | ||
| "moderate": 0, | ||
| "minor": 0 | ||
| }, | ||
| "safety_multiplier": 1.0 | ||
| }, | ||
| "catq": null, | ||
| "tus": null, | ||
| "signals": { | ||
| "escalations": 0, | ||
| "pushes": 0, | ||
| "posted": 0, | ||
| "steps_completed": 4 | ||
| } | ||
| }, | ||
| "trace": "pr4804.trace.json.gz", | ||
| "trace_bytes": 63929 | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| # Run report | ||
|
|
||
| - checkout: PR-TIME TREE (head 5d3e53170aae) — the tree matches the diff exactly; repo-wide greps DO reflect PR-time state | ||
|
|
||
| ## review_text | ||
|
|
||
| **Scan:** | ||
| | Category | Result | | ||
| |---|---| | ||
| | Tests / verification | 4 finding(s) below | | ||
| | Security | no finding reported | | ||
| | Docs / comments | 2 finding(s) below | | ||
| | Behavior / compatibility | 1 finding(s) below | | ||
| | Correctness | 5 finding(s) below | | ||
|
|
||
| **Validated:** | ||
| - [resolved] Abort slot leak — GPU generation runner now calls on_requests_finished; residual: NPU generation runner still missing same hook (npu_generation_model_runner.py:48-57 vs gpu_generation_model_runner.py:81-82). | ||
| - [resolved] talker2codec_raw put_req_chunk — emitted_any reads transfer_manager.put_req_chunk incremented by omni_connector_model_runner_mixin.py:1001-1002 on send. | ||
| - [resolved] Abort slot-leak concern addressed on CUDA generation runner; residual unchecked on NPU generation runner | ||
| - [resolved] NPU twin — npu_ar_model_runner already calls `on_requests_finished`; gpu_generation change mirrors pattern; NPU CI red unlikely introduced here | ||
| - [claim-verified] patch.py:1244-1251 — only caller of compute_seed_tts_wer_metrics; stereo env vars handled upstream in _pcm_s16le_to_seed_tts_wer_bytes | ||
| - [claim-refuted] PR body 'Fix some bugs: TODO' — incomplete; no enumerated bug-fix list at head. | ||
|
|
||
| `vllm_omni/model_executor/models/moss_tts/modeling_moss_tts_codec.py:~478` [major] — In `_decode_streaming_batch`, when all `codec_stream_slots` are busy and a streaming request's payload carries `finished=True` before `session.acquire()` succeeds, the code warns and calls `_finish_stream_request(..., None)`, which pops `_stream_pending_codes` without decoding (lines 477-484, 561). That request is omitted from `outputs`, so `audios[i]` stays `_empty_audio()` (lines 243, 360-362) while `forward()` still returns `OmniOutput`. With `codec_stream_slots: 8` and stage-1 `max_num_seqs: 8` in moss_tts_local.yaml, this is reachable under full concurrent load when slots are held across forwards. Prefer admission control, blocking wait, or a client-visible error over silent audio loss. (evidence: modeling_moss_tts_codec.py:477-484 `if finished: logger.warning(...); self._finish_stream_request(request_id, session, None); continue` — finish while starved drops codes and skips outputs; modeling_moss_tts_codec.py:243 `audios: list[torch.Tensor] = [empty] * num_req` and modeling_moss_tts_codec.py:360-362 only overwrite `audios[i]` from `_decode_streaming_batch` outputs; modeling_moss_tts_codec.py:561 `self._stream_pending_codes.pop(request_id, None)` clears buffered codes without decode; moss_tts_local.yaml:21 `codec_stream_slots: 8` and moss_tts_local.yaml:42 `max_num_seqs: 8`.) | ||
|
|
||
| ```suggestion | ||
| if finished: | ||
| raise RuntimeError( | ||
| f"MOSS codec stream request {request_id} finished before a codec " | ||
| f"stream slot became available; refusing to drop buffered codes." | ||
| ) | ||
| ``` | ||
|
|
||
| `vllm_omni/platforms/npu/worker/npu_generation_model_runner.py:~48` [major] — This PR wires `on_requests_finished` into `gpu_generation_model_runner._update_request_states` so `MossTTSCodecDecoder` releases codec stream slots on abort/disconnect; `npu_generation_model_runner._update_request_states` still only removes batch entries and never calls the hook, so MOSS codec stream slots and pending code buffers leak on NPU when requests finish outside the terminal streaming payload. Mirror the GPU hook before removing finished requests. (evidence: Added by this PR: gpu_generation_model_runner.py:81-82 `if scheduler_output.finished_req_ids and hasattr(self.model, "on_requests_finished"): self.model.on_requests_finished(scheduler_output.finished_req_ids)`. Unchanged NPU gap at npu_generation_model_runner.py:48-51 `def _update_request_states(self, scheduler_output: SchedulerOutput): # remove requests for req_id in scheduler_output.finished_req_ids: self.input_batch.remove_request(req_id)` — no `on_requests_finished` call. Sibling AR runners already invoke the hook: gpu_ar_model_runner.py:942-943 and npu_ar_model_runner.py:419-420. Consumer needing cleanup: modeling_moss_tts_codec.py:564-571 `def on_requests_finished(...) ... to avoid leaking stream slots and buffered codes.`) | ||
|
|
||
| Proposed change (not a ready patch): def _update_request_states(self, scheduler_output: SchedulerOutput): | ||
| if scheduler_output.finished_req_ids and hasattr(self.model, "on_requests_finished"): | ||
| self.model.on_requests_finished(scheduler_output.finished_req_ids) | ||
| for req_id in scheduler_output.finished_req_ids: | ||
|
|
||
| `vllm_omni/benchmarks/patch/patch.py:1250` [major] — In `async_request_openai_audio_speech`, the `except` around `_pcm_s16le_to_seed_tts_wer_bytes` stores raw `pcm_capture` (48 kHz stereo for Local-v1.5) but still leaves `success=True`. `compute_seed_tts_wer_metrics` then treats those bytes as 24 kHz mono (`_pcm_s16le_to_f32_16k` default), producing plausible-but-wrong WER. Mark the sample failed instead of storing mislabeled PCM. (evidence: patch.py:1249-1251 `except Exception as ex: logger.warning(...); output.tts_output_pcm_bytes = bytes(pcm_capture)` — fallback stores raw streamed PCM; patch.py:1260 `output.success = True` — success set regardless of normalization failure; patch.py:117-119 documents MOSS-TTS-Local-Transformer-v1.5 streams 48 kHz stereo PCM; seed_tts_eval.py:598 `wav_16k = _pcm_s16le_to_f32_16k(pcm)` — consumer assumes default 24000 Hz mono (seed_tts_eval.py:176 `pcm_sample_rate: int = 24000`).) | ||
|
|
||
| Proposed change (not a ready patch): except Exception as ex: | ||
| logger.error("Seed-TTS WER PCM normalization failed: %s", ex) | ||
| output.tts_output_pcm_bytes = None | ||
| output.success = False | ||
| output.error = f"pcm_normalization_failed: {ex}" | ||
|
|
||
| `vllm_omni/platforms/npu/worker/npu_generation_model_runner.py:~783` [major] — PR makes MossTTSCodecDecoder.forward require seq_token_counts; GPU gpu_generation_model_runner._dummy_run supplies it (line 799) but NPU npu_generation_model_runner._dummy_run (780–783) still omits it while passing padded input_ids, so Moss codec warmup/profile on NPU raises RuntimeError. (evidence: modeling_moss_tts_codec.py:260-265 `token_counts = self._normalize_seq_token_counts(kwargs.get("seq_token_counts"))` then `raise RuntimeError("MossTTS codec requires seq_token_counts; otherwise concatenated codec tokens cannot be split per request.")`; gpu_generation_model_runner.py:799 `model_kwargs["seq_token_counts"] = [int(num_tokens_padded)]`; unchanged NPU sibling at npu_generation_model_runner.py:780-783 only sets runtime_additional_information with no seq_token_counts assignment.) | ||
|
|
||
| Proposed change (not a ready patch): if hasattr(self.model, "get_dummy_runtime_additional_information"): | ||
| runtime_addi = self.model.get_dummy_runtime_additional_information(num_reqs) | ||
| model_kwargs["runtime_additional_information"] = runtime_addi | ||
| model_kwargs["seq_token_counts"] = [int(num_tokens_padded)] | ||
|
|
||
| `vllm_omni/model_executor/models/moss_tts/modeling_moss_tts_talker.py:1462` [major] — Local talker preprocess does `max_new_frames = info_dict.get('max_new_frames', [-1])[0]` without scalar normalization; a bare int raises TypeError on prefill. MossTTSDelayTalkerForGeneration (lines 471-477) still normalizes list/tuple/int — mirror that here. In-repo serving wraps the value as a one-element list, so this is latent for those paths but a regression from the prior Local preprocess block. (evidence: modeling_moss_tts_talker.py:1462 `max_new_frames = info_dict.get("max_new_frames", [-1])[0]` — subscripts without scalar guard; int input raises TypeError. modeling_moss_tts_talker.py:471-477 (unchanged Delay talker preprocess) `max_new_frames_req = info_dict.get("max_new_frames")` then list/tuple unwrap and `int(...)` with TypeError fallback. serving_speech.py:1993-1994 `params["max_new_frames"] = [request.max_new_tokens]` — in-repo online path passes a list, not bare int.) | ||
|
|
||
| Proposed change (not a ready patch): max_new_frames_req = info_dict.get("max_new_frames") | ||
| if isinstance(max_new_frames_req, (list, tuple)) and max_new_frames_req: | ||
| max_new_frames = max_new_frames_req[0] | ||
| else: | ||
| max_new_frames = max_new_frames_req if max_new_frames_req is not None else -1 | ||
|
|
||
| `vllm_omni/model_executor/stage_input_processors/moss_tts.py:225` [minor] — moss_tts_local (and moss_tts_realtime) route Stage 0 through new talker2codec_raw_async_chunk with initial_codec_chunk_frames, codec_streaming meta, and a finish sentinel (code_flat_numel=0), but no tests reference this processor—only tts_local_seed renames in test_omni_gpu_model_runner.py. Add CPU core_model tests for first-chunk threshold, steady chunks, and finish-sentinel behavior. (evidence: pipeline.py:94 `async_chunk_process_next_stage_input_func=(f"{_PROC}.talker2codec_raw_async_chunk")` under MOSS_TTS_LOCAL_PIPELINE; moss_tts.py:225 `def talker2codec_raw_async_chunk(`; moss_tts.py:271-272 `initial_chunk_frames = int(cfg.get("initial_codec_chunk_frames") or 0)`; moss_tts.py:300-301 `code_flat_numel=0,` / `codec_streaming=True,`; repo grep: zero matches for talker2codec_raw_async_chunk or talker2codec under tests/; test_omni_gpu_model_runner.py:278 `extra_args={"tts_local_seed": 42},`) | ||
|
|
||
| `vllm_omni/entrypoints/openai/serving_speech.py:3715` [minor] — This PR removes the qwen3_tts-only gate and writes tts_local_seed into stage-0 extra_args whenever request.seed is set. gpu_model_runner._talker_mtp_forward consumes tts_local_seed for every talker_mtp model (qwen3_tts, fish_speech, moss_tts_local, qwen3_omni), so HTTP seed now drives talker-local sampling for backends beyond the MOSS-local path without an explicit per-backend contract. Scope the write to consumers (e.g. qwen3_tts, moss_tts_local) or document the cross-backend contract. (evidence: serving_speech.py:3707 `if request.seed is not None and sampling_params_list:` — no _tts_model_type guard; serving_speech.py:3715 `stage0_params.extra_args["tts_local_seed"] = request.seed` — unconditional write. Unchanged consumer in PR-time tree: gpu_model_runner.py:1826 `seed = extra_args.get("tts_local_seed")` inside _talker_mtp_forward, which runs only when the model exposes talker_mtp (grep: qwen3_tts, fish_speech, moss_tts_local, qwen3_omni).) | ||
|
|
||
| ```suggestion | ||
| if self._tts_model_type in ("qwen3_tts", "moss_tts_local"): | ||
| if stage0_params.extra_args is None: | ||
| stage0_params.extra_args = {} | ||
| stage0_params.extra_args["tts_local_seed"] = request.seed | ||
| ``` | ||
|
|
||
| **Additional observations (beyond the comment budget):** | ||
| - `general` [minor] Abort slot-leak cleanup is wired on the CUDA generation runner (`gpu_generation_model_runner._update_request_states`); the NPU generation runner still lacks the matching `on_requests_finished` hook. | ||
| - `vllm_omni/model_executor/models/moss_tts/modeling_moss_tts_codec.py:729` [minor] `_build_codec` always instantiates vendored v2 via `MossAudioTokenizerV2Config.from_pretrained` with no checkpoint-version gate; legacy is only reached on exception. Both config classes share `model_type='moss-audio-tokenizer'` and strip legacy `model_type` from JSON, so config load can succeed for either layout and mismatch surfaces only in strict `load_weights`. Consider gating on architecture-discriminating fields (sampling_rate/downsample_rate/number_channels or weight key patterns), not `model_type` alone. | ||
| - `vllm_omni/model_executor/models/moss_tts/modeling_moss_tts_talker.py:1505` [minor] MOSS Local per-frame generation now runs in `talker_mtp` and mutates per-request `req_infos`; `_talker_mtp_forward` only forwards `req_infos` when `talker_mtp_accepts_req_infos` is true (production Moss Local sets it). Existing `CaptureTalkerMTP` tests omit both the flag and a `req_infos` parameter, so this runner↔talker contract is untested. | ||
| - `tests/model_executor/models/moss_tts/test_per_request_codes.py:71` [minor] `test_per_request_codes.py` guards MOSS-TTS per-request codec routing but has no `pytestmark`; Buildkite runs `tests/model_executor` with `-m 'core_model and cpu'`, so these tests are deselected. Add `pytestmark = [pytest.mark.core_model, pytest.mark.cpu]` (see `test_talker_logits_processor.py`). | ||
|
|
||
| **Verdict:** FOLLOW-UP REQUIRED (post-merge) |
Binary file not shown.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| { | ||
| "wall_s": 542.2, | ||
| "split": "train", | ||
| "rc": 0, | ||
| "run_dir": "/data/zhoutaichang/copilot/InferMatrixCopilot/eval/dataset/arms/copilot_v20cb_train/runs/pr4817/run-20260817-194337-778499", | ||
| "recorded_at": "2026-08-17T19:52:37", | ||
| "attempts": [ | ||
| { | ||
| "invocation_id": "inv-580b823f", | ||
| "outcome": "run:run-20260817-194337-778499", | ||
| "rc": 0, | ||
| "pre_run": null | ||
| }, | ||
| { | ||
| "run_dir": "run-20260817-194337-778499", | ||
| "llm_calls": 0, | ||
| "input_tokens": 0, | ||
| "output_tokens": 0, | ||
| "usd": 0.0, | ||
| "source": "no-trace" | ||
| } | ||
| ], | ||
| "attempt_usd_total": 0.0, | ||
| "llm_calls": 0, | ||
| "input_tokens": 0, | ||
| "output_tokens": 0, | ||
| "usd": 0.0, | ||
| "source": "no-trace", | ||
| "metrics": { | ||
| "schema": 1, | ||
| "run": "run-20260817-194337-778499", | ||
| "task_kind": "pr_review", | ||
| "status": "done", | ||
| "quality": { | ||
| "q": null, | ||
| "weight_coverage": 0.0, | ||
| "partial": true, | ||
| "abstained": false, | ||
| "components": { | ||
| "recall_w": null, | ||
| "precision_v": null, | ||
| "useful": null, | ||
| "calib": null, | ||
| "decision": null | ||
| } | ||
| }, | ||
| "cost": { | ||
| "usd": 0.1656, | ||
| "minutes": 8.99, | ||
| "ci_minutes": 0, | ||
| "input_tokens": 402875, | ||
| "output_tokens": 51647, | ||
| "tool_calls": 172, | ||
| "usd_ref": 1.0, | ||
| "min_ref": 10.0, | ||
| "cost_index": 1.3637, | ||
| "cost_partial": false, | ||
| "unpriced_calls": 0, | ||
| "source": "events", | ||
| "llm_calls": null, | ||
| "by_role": null, | ||
| "cache_read_tokens": null, | ||
| "cache_creation_tokens": null | ||
| }, | ||
| "timings": { | ||
| "steps": { | ||
| "fetch": 4.72, | ||
| "gate": 2.24, | ||
| "review": 532.45, | ||
| "report": 0.11 | ||
| }, | ||
| "time_to_first_result_s": 539.46 | ||
| }, | ||
| "risk": { | ||
| "incidents": { | ||
| "catastrophic": 0, | ||
| "severe": 0, | ||
| "moderate": 0, | ||
| "minor": 0 | ||
| }, | ||
| "safety_multiplier": 1.0 | ||
| }, | ||
| "catq": null, | ||
| "tus": null, | ||
| "signals": { | ||
| "escalations": 0, | ||
| "pushes": 0, | ||
| "posted": 0, | ||
| "steps_completed": 4 | ||
| } | ||
| }, | ||
| "trace": "pr4817.trace.json.gz", | ||
| "trace_bytes": 19765 | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All 15 new cost records set
attempt_usd_totaland the top-level token counts to zero even though their nestedmetrics.costcontains nonzero event-derived values (this item records $0.5684 and 1,690,401 input tokens).eval/dataset/aggregate_costs.pytreats a presentattempt_usd_totalas authoritative and reads tokens only from the top level, so aggregating these retained arms reports $0 and zero tokens rather than $2.6971 for train and $1.4340 for validation, corrupting cost and cost-per-success comparisons. Populate these fields from the recorded metrics, or omit unavailable totals so the aggregator's metrics fallback is used.Useful? React with 👍 / 👎.