Agentic Inference updates#314
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for multi-turn conversation benchmarking, including a new MultiTurnDataset class, a MultiTurnStrategy for turn sequencing, and a ConversationManager for state tracking. It also provides extensive documentation, validation schemas, and utility scripts for dataset conversion and analysis. Feedback focuses on optimizing performance and memory efficiency within the MultiTurnDataset class, specifically recommending the use of vectorized pandas operations to avoid memory-intensive dictionary conversions and suggesting an optimization to reduce the algorithmic complexity of building message histories from quadratic to linear.
Several OpenAI-compatible servers (notably SGLang and vLLM with their reasoning/tool parsers) drift from the strict OpenAI spec in ways the existing decoders silently swallowed: * SSEDelta.reasoning was the wrong field name. SGLang/vLLM emit `reasoning_content` on streaming deltas (matching the non-streaming ChatCompletionResponseMessage). The previous code never matched, so every reasoning chunk on a thinking-mode response was dropped on the floor with no error. * SSEDelta fields default to None (servers send `null`, not `""`). Previous defaults of `""` looked truthy when the server actually had no payload for that channel in a given chunk. * ChatCompletionResponseMessage / ChatCompletionChoice / ChatCompletionResponse: many fields (`content`, `refusal`, `finish_reason`, `usage`, `system_fingerprint`) are not always emitted. Without defaults, msgspec rejected non-streaming responses from these servers entirely. * decode_sse_message now catches msgspec.ValidationError, logs the raw chunk preview, and returns None so the stream keeps draining. The prior behaviour bubbled into the worker's outer Exception handler and lost the diagnostic. TextModelOutput now carries `tool_calls` and `finish_reason` as first-class fields (in addition to the existing metadata copy) so multi-turn replay can build history without reaching into metadata. A diagnostic `chunk_stats` dict counts content / reasoning / tool-call chunks per response for replay-determinism debugging. `array_like=False` on TextModelOutput is required to safely add fields without breaking the positional wire layout. PromptData and ErrorData flip too for consistency.
Two correctness changes for multi-turn replay against thinking-mode and tool-using models: 1. reasoning_content propagation. Prior assistant turns must replay their thinking trace as part of the message history; without it, the chat-template-rendered prompt diverges from what the original capture sent and outputs differ from the captured trajectory even at temperature=0. 2. tools propagation across all turns. Every request must carry the same tools array as turn 1; SGLang's tool-call parser is gated on a non-empty `tools` field in the request, and turns that omit it silently bypass the parser, leaking literal tool-call markup into the assistant `content` channel. New optional cache-bursting salt (multi_turn.enable_salt: bool, default False) appends a per-conversation blake2b digest to the end of each trajectory's system prompt. This keeps within-trajectory prefix caching intact while preventing cross-trajectory KV-cache leak during replay of datasets that share a long system prompt across many trajectories. The salt is computed once per conversation_id and reused on every turn of that conversation. apply_salt is idempotent on already-salted prompts so re-runs are stable. See examples/09_MultiTurn/docs for the full methodology and validation; not included in this commit. Mechanical change in dataset.py: load_from_file gains a **dataset_kwargs passthrough so the factory can forward enable_salt without expanding the signature for every future option.
The customer_support example and its two near-duplicate config files predated the agentic datasets and no longer earn their keep: - multi_turn_benchmark.yaml and multi_turn_with_concurrency.yaml differ only in `name`, two inline comments, and `report_dir`. Both already set target_concurrency: 32 — the README's Concurrency Control section documents the same knob inline. Keeping a second YAML to teach a setting that's already in the first is noise. - customer_support_conversations.jsonl was the toy dataset backing those YAMLs. With them gone it has no consumer. The remaining agentic YAMLs are tuned to actually work as written: - model_params.name: "/model" matches SGLang's --model-path mount - temperature: 0 (greedy) per MLPerf-inference reproducibility convention - max_new_tokens sized to the longest observed turn in each capture - target_concurrency reduced from theoretical maxima to values that match real B200-class server capacity on a 1T MoE - client.warmup_connections: 0 / max_idle_time: 0.5 work around uvicorn closing pre-warmed idle sockets after 5s, which otherwise causes every first request to fail with ConnectionResetError README updated: Basic Configuration example uses agentic_coding / agentic_coding_flat.jsonl (the dataset that's actually in scope post- deletion); Using Configuration File invokes agentic_coding_benchmark.yaml; Example Datasets section dropped (the only entry was customer_support).
…alysis utility - score_inline_accuracy.py: single-script scorer for multi-turn benchmark runs. Coding turns score by multiset IoU on a curated whitelist of ~40 canonical bash exes; workflow turns score by exact-match on `intent: IXXX`. Folds in events.jsonl -> model_assistants.jsonl conversion so a benchmark report dir can be scored in one call. - analyze_flat_jsonl.py: produces a single composite summary plot (turns/conv, ISL/OSL distributions, per-turn growth, token-class violins) for any flat multi-turn JSONL.
…ance - MultiTurnConfig: add `enable_salt: bool = False` knob. - MultiTurnDataset: when enabled, append `\n\n[cache_salt: <hex>]` to the system message once per trajectory, where hex is `blake2b(conversation_id, digest_size=8).hexdigest()`. Same salt is reused across all turns of one trajectory so within-trajectory prefix caching is preserved; differs across trajectories so the cross-trajectory cache match terminates at the salt boundary. - MultiTurnDataset: drop rows with no `conversation_id` after load (e.g. the `_type: dataset_metadata` license/source sentinel some upstream snapshots prepend). Methodology + full-dataset salt-vs-no-salt accuracy comparison in examples/09_MultiTurn/docs/EVALUATION.md.
- README: new "Accuracy Evaluation" subsection under "Running Multi-Turn Benchmarks" documenting the score_inline_accuracy.py command and what it writes into report_dir. Completes the convert -> replay -> score pipeline. - score_inline_accuracy.py: ruff-format/lint pass (formatting only, no behavior change).
- Add ModelParams.chat_template_kwargs (forwarded per request to
vLLM/SGLang). Enables Kimi-K2.6 Thinking-mode by sending
{'thinking': True, 'preserve_thinking': True} on every request so
prior assistant reasoning is rendered back into the prompt as
<think>...</think> blocks during multi-turn replays.
- Wire field through ChatCompletionRequest msgspec struct and
openai_msgspec_adapter so it lands on the wire as a top-level
request key.
- Score workflow accuracy from the structured intent_codes set
stamped on each scorable assistant row (no regex fallback): a turn
is a hit iff the model's extracted I### code is a member of the
per-turn ground-truth set. Tool-only assistant turns remain
unscorable by design.
- inject_tool_delay knob on multi_turn dataset config: when set, the
strategy defers the next turn issue by the dataset's embedded
delay_seconds via loop.call_later, modelling the producer-side
tool/human pause that the original capture saw.
- Update agentic_{coding,workflow}_benchmark.yaml to point at
the t1 datasets, T=1.0/top_p=0.95 sampling, salt enabled, and
chat_template_kwargs preset for Kimi-K2.6.
- Update README + score_inline_accuracy.py + JSONL schema to match
the new _t1.jsonl naming and intent_codes/delay_seconds fields.
- Regenerate full templates to expose the new schema field.
Tests cover end-to-end chat_template_kwargs propagation, the
intent_codes scoring rule, and the inject_tool_delay scheduling
path.
hvagadia
force-pushed
the
hvagadia/cache-salt-and-correctness
branch
from
66f0a3d to
1bbc7ab
Compare
hvagadia
changed the title
hvagadia
changed the title
| @@ -0,0 +1,525 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
| """Inline accuracy scorer for multi-turn benchmark runs. | |||
There was a problem hiding this comment.
The name of the file needs to be more explicit (and aligned with this summary)
| Coding turn: IoU of the multisets of canonical bash exes used in the turn | ||
| Workflow turn: 1 if `intent: IXXX` code matches gt, else 0 |
There was a problem hiding this comment.
Should these be 2 separate scorer? IIRC the design was to have modularized accuracy checker for different components, so they can be reused for different dataset (e.g. if we have a longer coding dataset, we can reuse this for that)
| # Canonical exe whitelist. Synonyms collapse to one canonical name. | ||
| # Anything not in this map is dropped. cd / pwd / echo are intentionally | ||
| # absent: they're navigation/output verbs, not action classes. | ||
| EXE_MAP: dict[str, str] = { |
There was a problem hiding this comment.
Does this map haave an original reference (paper/code)? Good to put a reference if it's Aapache2.0 license
| multi_turn: | ||
| turn_timeout_s: 600.0 | ||
| enable_salt: true # add salt after system prompt to prevent cache reuse across trajectories | ||
| inject_tool_delay: true # add delay before user/tool turns |
There was a problem hiding this comment.
(I might have missed the prev PR) is the tool delay injected dynamically (calced based on distribution), or statically (embedded in the dataset)?
(Same Q for the salt as well)
There was a problem hiding this comment.
tool delay is embedded in the dataset from the proposed distribution and used statically by client. Salt is deterministically computed at runtime.
nv-alicheng
left a comment
nv-alicheng
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Reviewed by: Codex + Claude | Depth: thorough
Found 11 issues across 7 files (3 couldn't be posted inline — see summary comment).
|
|
||
| idx, turn = pair | ||
| idx, turn = turns[cursor] | ||
| self._active_iters[conv_id] = (turns, cursor + 1) |
There was a problem hiding this comment.
[Codex+Claude] high (bug): Cursor is advanced to cursor + 1 at this line before the scheduled delay executes.
If a timeout on the preceding in-flight turn triggers _abort_remaining_turns, it reads the already-advanced cursor (line 417: turns, cursor = state) and iterates turns[cursor:] — skipping the turn whose delay was just cancelled. mark_turn_failed and register_skipped/_publish_synthetic_failure are never called for that turn, leaving ConversationManager's completion count off by one and the benchmark missing the failure event entirely.
| "n_scorable": n, | ||
| "n_perfect": n_perfect, | ||
| "n_zero": n_zero, | ||
| "pass_rate": round(total / n, 4) if n else float("nan"), |
There was a problem hiding this comment.
[Codex+Claude] high (data-integrity): float("nan") is serialized as the bare token NaN by json.dumps (line 518) without allow_nan=False.
NaN is not valid JSON per RFC 8259 — jq, Go's encoding/json, and most strict parsers will refuse to read the output file. Replace float("nan") with None so the field becomes null in JSON, which is valid and clearly signals no data.
| i += 1 | ||
| if i >= len(tokens): | ||
| return None | ||
| leaf = _PATH_LEAF.search(tokens[i]).group(0).lower() |
There was a problem hiding this comment.
[Claude] medium (bug): _PATH_LEAF.search(tokens[i]).group(0) raises AttributeError when tokens[i] is exactly "/" (a bare slash).
re.compile(r"[^/]+$") returns None for "/" because there are no non-slash characters before $. This aborts the entire scoring run. Guard against it:
m = _PATH_LEAF.search(tokens[i])
if m is None:
return None
leaf = m.group(0).lower()| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _build_index_to_key(gt_jsonl: Path) -> list[tuple[str, int]]: |
There was a problem hiding this comment.
[Claude] medium (data-integrity): _build_index_to_key silently produces wrong results if the gt JSONL has a different turn count or order than the benchmark run dataset.
The positional index list is used to map sample_idx_map.json integer indices to (conv_id, turn) keys. If the gt file has extra or missing conversations, all subsequent UUID-to-key mappings past the mismatch point attribute model outputs to the wrong turns — producing incorrect scores with no warning. At minimum, assert or warn when len(index_to_key) differs from the number of UUIDs in uuid_to_index.
| try: | ||
| payload = await request.json() | ||
| received_payloads.append(payload) | ||
| except Exception: |
There was a problem hiding this comment.
[Claude] low (error-handling): except Exception silently swallows request-parse failures.
logger = None # noqa: F841 suppresses the ruff warning but provides zero signal when parsing fails. Per project rules (AGENTS.md), every except block must log or include a comment explaining why the error is ignored. A parse failure here would leave received_payloads empty with no diagnostic output, making failures very hard to debug.
| logit_bias: dict[str, float] | None = None | ||
| user: str | None = None | ||
| chat_template: str | None = None | ||
| chat_template_kwargs: dict[str, Any] | None = None |
There was a problem hiding this comment.
[Claude] low (design): The gc=False AT-RISK audit comment at line 112 lists messages, tools, and logit_bias but omits the newly added field at this line.
chat_template_kwargs: dict[str, Any] | None is also a mutable container. While the current Kimi use-case values are dict[str, bool] (cycle-free), the Any type admits cyclic references in general. Add chat_template_kwargs to the audit comment so future reviewers know it was considered.
| if val and isinstance(val, str): | ||
| system_content = val | ||
| break | ||
| if self._enable_salt and system_content: |
There was a problem hiding this comment.
[Claude] low (design): Salt is silently skipped for conversations that have no system prompt (system_content is None).
if self._enable_salt and system_content: means that when enable_salt=True is requested, conversations without a system row receive no cache differentiation — partially defeating the purpose of the flag. At minimum, log a warning at INFO level identifying which conversations were skipped so the caller knows they are unprotected.
| def _build_index_to_key(gt_jsonl: Path) -> list[tuple[str, int]]: | ||
| keys: list[tuple[str, int]] = [] | ||
| by_conv: dict[str, list[dict]] = {} | ||
| for line in gt_jsonl.read_text().splitlines(): |
There was a problem hiding this comment.
[Claude] low (performance): gt_jsonl.read_text().splitlines() loads the entire ground-truth file into memory as a single string before splitting.
For large benchmark datasets this doubles peak memory vs. streaming. _iter_assistant_turns (line 268) already uses the correct pattern — iterate the open file handle line-by-line:
with open(gt_jsonl) as fh:
for line in fh:
...
Review Council — Multi-AI Code ReviewReviewed by: Codex + Claude | Depth: thorough Found 11 issues across 7 files.
🔴 Must Fix (critical/high)Issues that will cause hangs, data loss, or incorrect behavior in production.
🟡 Should Fix (medium)Real issues that produce incorrect results under reachable conditions.
🔵 Consider (low)Valid improvements, suitable as follow-ups.
🤖 Generated by |
|
|
||
|
|
||
| def _extract_intent_code(turn: dict) -> str | None: | ||
| """Two-stage extractor: |
There was a problem hiding this comment.
any OSS implementations / papers / conventions we could link to for this section?
| # Mandatory: with the default warmup behaviour, every request fails with | ||
| # ConnectionResetError because uvicorn closes pre-warmed idle sockets after 5s. | ||
| client: | ||
| warmup_connections: 0 |
There was a problem hiding this comment.
whats the http error code ur seeing on this?
dont remember seeing this in the past with sglang.
(i expect tcp-keepalive is disabled (default))
also max-idle-time should is likely not making a difference here, lets use default if we can.
| # Event-driven state — populated in execute(). | ||
| self._pending_convs: deque[tuple[str, list[tuple[int, int]]]] = deque() | ||
| self._active_iters: dict[str, Iterator[tuple[int, int]]] = {} | ||
| self._active_iters: dict[str, tuple[list[tuple[int, int]], int]] = {} |
There was a problem hiding this comment.
lets add a typedef/alias, hard to understand what tuple[list[tuple[int, int]], int] is supposed to mean here and in self._pending_convs as well?
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Claude-only review (Codex unavailable — bwrap/pivot_root not permitted in this environment). Depth: thorough.
| num_repeats=num_repeats, | ||
| ) | ||
| if config.multi_turn is not None and config.multi_turn.enable_salt: | ||
| assert isinstance(dataloader, MultiTurnDataset) |
There was a problem hiding this comment.
[Claude] high (bug): assert isinstance(dataloader, MultiTurnDataset) raises AssertionError — not a user-facing error — when enable_salt=True but the loader is not a MultiTurnDataset. Worse, the assert is silently skipped under python -O, leading to AttributeError on dataloader.enable_salt() with no diagnostic. The project convention for config-validation failures is InputValidationError. Replace with:
if not isinstance(dataloader, MultiTurnDataset):
raise InputValidationError(
"multi_turn.enable_salt requires a multi-turn dataset, got "
f"{type(dataloader).__name__}"
)| handle = self._loop.call_later( | ||
| delay, self._issue_turn_now, conv_id, idx, turn | ||
| ) | ||
| self._delay_handles[conv_id] = handle |
There was a problem hiding this comment.
[Claude] high (concurrency): The early-exit guard in execute() (a few lines before this hunk) reads if not self._active_iters and not self._inflight. This guard fires incorrectly when every conversation has its first turn delayed: _issue_next_turn advances the cursor and stores the handle here, but returns before populating _inflight. If all _active_iters slots are occupied by pending-delay conversations, the guard sees _inflight empty and never sets _all_done, causing execute() to hang at await self._all_done.wait(). The guard should also check not self._delay_handles:
if not self._active_iters and not self._inflight and not self._delay_handles:
return phase_issuer.issued_countSimilarly, _fill_slot (further below) checks elif not self._active_iters to set _all_done — this will fire prematurely while delays are pending, terminating the run before delayed turns execute.
| "name", | ||
| "tool_calls", | ||
| "tool_results", | ||
| "reasoning_content", |
There was a problem hiding this comment.
[Claude] medium (api-contract): reasoning_content is unconditionally forwarded in prior-assistant-turn history messages. This field is a Kimi/SGLang-specific extension and is not part of the OpenAI Chat Completions API spec for request message objects. Any server that validates input strictly (standard vLLM, OpenAI-compatible endpoints, TRT-LLM serve) will reject requests with a 400/422 error when prior assistant turns carry this field.
Since ChatMessage.reasoning_content has omit_defaults=True and defaults to None, the field is only included when the dataset row has a non-null value — so this only triggers for datasets recorded from Kimi runs. But there is no api_type gate or config option to strip it, and no documentation warning. Recommend gating on api_type in (APIType.OPENAI_MSGSPEC, APIType.SGLANG) or adding a strip_reasoning_from_history option to MultiTurnConfig.
| data = _parse_model_output(ev.get("data")) | ||
| if data is None: | ||
| continue | ||
| conv_id, client_turn = key |
There was a problem hiding this comment.
[Claude] medium (data-integrity): derive_model_assistants assigns assistant turn number as client_turn + 1 (the turn number of the preceding user/tool row + 1). This works only because _validate_turn_numbering enforces that every client turn is immediately followed by an assistant turn at the next sequential turn number. That invariant is not visible at this call site.
If a caller passes a JSONL file from a non-validating source (e.g. a manually constructed or future-format dataset with non-contiguous turn numbers), all turns are silently scored against the wrong ground-truth rows — missing_in_model will equal the full dataset size and pass rate will be 0 with no error or warning. A comment citing the coupling to _validate_turn_numbering, or a runtime assertion that client_turn + 1 exists in the gt index, would prevent silent misscoring.
| if args.report_dir is not None: | ||
| model_path = args.report_dir / "model_assistants.jsonl" | ||
| derive_model_assistants(args.report_dir, args.gt, model_path, args.dataset_name) | ||
| else: |
There was a problem hiding this comment.
[Claude] low (error-handling): When --model is passed directly (not --report-dir), model_path = args.model is used without checking whether the file exists. score_run then calls _iter_assistant_turns(model_jsonl) which raises a raw FileNotFoundError with a Python traceback. The --report-dir path is consistent but the --model path should add the same guard:
if not model_path.exists():
sys.exit(f"model file not found: {model_path}")
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Claude-only review (Codex unavailable — bwrap/pivot_root not permitted in this environment). Depth: thorough.
| num_repeats=num_repeats, | ||
| ) | ||
| if config.multi_turn is not None and config.multi_turn.enable_salt: | ||
| assert isinstance(dataloader, MultiTurnDataset) |
There was a problem hiding this comment.
[Claude] high (bug): assert isinstance(dataloader, MultiTurnDataset) raises AssertionError — not a user-facing error — when enable_salt=True but the loader is not a MultiTurnDataset. Worse, the assert is silently skipped under python -O, leading to AttributeError on dataloader.enable_salt() with no diagnostic. The project convention for config-validation failures is InputValidationError. Replace with:
if not isinstance(dataloader, MultiTurnDataset):
raise InputValidationError(
"multi_turn.enable_salt requires a multi-turn dataset, got "
f"{type(dataloader).__name__}"
)| handle = self._loop.call_later( | ||
| delay, self._issue_turn_now, conv_id, idx, turn | ||
| ) | ||
| self._delay_handles[conv_id] = handle |
There was a problem hiding this comment.
[Claude] high (concurrency): The early-exit guard in execute() (a few lines before this hunk) reads if not self._active_iters and not self._inflight. This guard fires incorrectly when every conversation has its first turn delayed: _issue_next_turn advances the cursor and stores the handle here, but returns before populating _inflight. If all _active_iters slots are occupied by pending-delay conversations, the guard sees _inflight empty and never sets _all_done, causing execute() to hang at await self._all_done.wait(). The guard should also check not self._delay_handles:
if not self._active_iters and not self._inflight and not self._delay_handles:
return phase_issuer.issued_countSimilarly, _fill_slot (further below) checks elif not self._active_iters to set _all_done — this will fire prematurely while delays are pending, terminating the run before delayed turns execute.
| "name", | ||
| "tool_calls", | ||
| "tool_results", | ||
| "reasoning_content", |
There was a problem hiding this comment.
[Claude] medium (api-contract): reasoning_content is unconditionally forwarded in prior-assistant-turn history messages. This field is a Kimi/SGLang-specific extension and is not part of the OpenAI Chat Completions API spec for request message objects. Any server that validates input strictly (standard vLLM, OpenAI-compatible endpoints, TRT-LLM serve) will reject requests with a 400/422 error when prior assistant turns carry this field.
Since ChatMessage.reasoning_content has omit_defaults=True and defaults to None, the field is only included when the dataset row has a non-null value — so this only triggers for datasets recorded from Kimi runs. But there is no api_type gate or config option to strip it, and no documentation warning. Recommend gating on api_type in (APIType.OPENAI_MSGSPEC, APIType.SGLANG) or adding a strip_reasoning_from_history option to MultiTurnConfig.
| data = _parse_model_output(ev.get("data")) | ||
| if data is None: | ||
| continue | ||
| conv_id, client_turn = key |
There was a problem hiding this comment.
[Claude] medium (data-integrity): derive_model_assistants assigns assistant turn number as client_turn + 1 (the turn number of the preceding user/tool row + 1). This works only because _validate_turn_numbering enforces that every client turn is immediately followed by an assistant turn at the next sequential turn number. That invariant is not visible at this call site.
If a caller passes a JSONL file from a non-validating source (e.g. a manually constructed or future-format dataset with non-contiguous turn numbers), all turns are silently scored against the wrong ground-truth rows — missing_in_model will equal the full dataset size and pass rate will be 0 with no error or warning. A comment citing the coupling to _validate_turn_numbering, or a runtime assertion that client_turn + 1 exists in the gt index, would prevent silent misscoring.
| if args.report_dir is not None: | ||
| model_path = args.report_dir / "model_assistants.jsonl" | ||
| derive_model_assistants(args.report_dir, args.gt, model_path, args.dataset_name) | ||
| else: |
There was a problem hiding this comment.
[Claude] low (error-handling): When --model is passed directly (not --report-dir), model_path = args.model is used without checking whether the file exists. score_run then calls _iter_assistant_turns(model_jsonl) which raises a raw FileNotFoundError with a Python traceback. The --report-dir path is consistent but the --model path should add the same guard:
if not model_path.exists():
sys.exit(f"model file not found: {model_path}")
| num_repeats=num_repeats, | ||
| ) | ||
| if config.multi_turn is not None and config.multi_turn.enable_salt: | ||
| assert isinstance(dataloader, MultiTurnDataset) |
There was a problem hiding this comment.
[Claude] high (bug): assert isinstance(dataloader, MultiTurnDataset) raises AssertionError when enable_salt=True but the loader is not a MultiTurnDataset. The assert is silently skipped under python -O, leading to AttributeError on dataloader.enable_salt() with no diagnostic. The project convention for config-validation failures is InputValidationError:
if not isinstance(dataloader, MultiTurnDataset):
raise InputValidationError(
"multi_turn.enable_salt requires a multi-turn dataset, got "
f"{type(dataloader).__name__}"
)
| handle = self._loop.call_later( | ||
| delay, self._issue_turn_now, conv_id, idx, turn | ||
| ) | ||
| self._delay_handles[conv_id] = handle |
There was a problem hiding this comment.
[Claude] high (concurrency): The early-exit guard in execute() reads if not self._active_iters and not self._inflight. When every conversation has its first turn delayed, _issue_next_turn advances the cursor and stores the handle here, but returns before populating _inflight. The guard then sees _inflight empty and _all_done is never set, causing execute() to hang at await self._all_done.wait(). Fix: also check not self._delay_handles.
The same issue affects _fill_slot: elif not self._active_iters fires prematurely while delays are pending, terminating the run before delayed turns execute.
| "name", | ||
| "tool_calls", | ||
| "tool_results", | ||
| "reasoning_content", |
There was a problem hiding this comment.
[Claude] medium (api-contract): reasoning_content is unconditionally forwarded in prior-assistant-turn history messages. This is a Kimi/SGLang-specific extension, not in the OpenAI Chat Completions spec for request message objects. Servers that validate input strictly (standard vLLM, TRT-LLM serve) will reject with 400/422 when prior assistant turns carry this field. Since ChatMessage.reasoning_content defaults to None and uses omit_defaults=True, this only triggers for datasets recorded from Kimi runs — but there is no api_type gate, config option to strip it, or documentation warning.
| data = _parse_model_output(ev.get("data")) | ||
| if data is None: | ||
| continue | ||
| conv_id, client_turn = key |
There was a problem hiding this comment.
[Claude] medium (data-integrity): client_turn + 1 works only because _validate_turn_numbering enforces sequential turn numbering — an invariant not visible here. If called with a non-validating source (manually constructed dataset, future format with non-contiguous turn numbers), all turns are silently scored against the wrong ground-truth rows: missing_in_model equals the full dataset size, pass rate is 0, with no error. A comment or runtime assertion would prevent silent misscoring.
| if args.report_dir is not None: | ||
| model_path = args.report_dir / "model_assistants.jsonl" | ||
| derive_model_assistants(args.report_dir, args.gt, model_path, args.dataset_name) | ||
| else: |
There was a problem hiding this comment.
[Claude] low (error-handling): When --model is passed directly, model_path = args.model is used without checking existence. score_run raises a raw FileNotFoundError with a traceback. The --report-dir path is consistent — the --model path needs the same existence check before use.
Review Council — Multi-AI Code ReviewReviewed by: Claude (Codex unavailable — bwrap/pivot_root not permitted in this environment) | Depth: thorough | PR: #314 "Agentic Inference updates" Found 5 new issues posted as inline comments (3 previously covered by earlier review pass were deduplicated). Commit hygiene: 14 commits, 6 chore/fix — consider squashing before merge. 🔴 Must Fix (critical/high)
🟡 Should Fix (medium)
🔵 Consider (low)
|
5 participants
What does this PR do?
Type of change
Related issues
Testing
Checklist