Skip to content

fix(models): compute usage totals, and reject bad builder defaults at bind time - #100

Merged
ethan-scitix merged 5 commits into
mainfrom
fix/model-record-hardening
Aug 14, 2026
Merged

fix(models): compute usage totals, and reject bad builder defaults at bind time#100
ethan-scitix merged 5 commits into
mainfrom
fix/model-record-hardening

Conversation

@ethan-scitix

@ethan-scitix ethan-scitix commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Type

  • fix — bug fix or alignment correction

Summary

Four fixes to what a per-call record may contain, and to when a call is worth failing over.

  • Usage totals are computed, not read. One concept had three stances: openai_chat raised when the reported total did not decompose, openai_completions accepted whatever the server sent, and the sglang transport never read a reported total at all — it summed. Both dialects now agree with sglang.
  • default_params rejects values that cannot round-trip. _named_json_value took any non-str/bytes Iterable, where the same-package copy_json_value has always taken only list/tuple. Narrowed to match.
  • The same rule now holds at call time. _kwargs_to_request pops stop before the JSON check runs and handles it with its own isinstance(stop, Iterable) branch, so narrowing _named_json_value closed the bind-time door and left the call-time one open.
  • Builder defaults are checked at bind time. meta() runs per response, so it was the first thing to notice a bad default — raising only after a call had been billed.

Why the usage check was the risky one. OutputContractError is documented as "a successful wire reply violated its declared output contract" — the tokens were billed. It is never caught anywhere in sieval, so it reaches the runner's generic except Exception, consumes a retry, and lands the sample in fails scoring 0. A server one token off discarded a good reply, and in the streaming path only after the rollout had finished. Computing the total also makes the identity structural, so _validate_input_scoring_boundary's total-token comparison can no longer fire and is removed; the remaining boundary checks compare usage against echoed logprob positions and still have teeth.

Why the Iterable gap mattered. Two admitted shapes corrupt ModelMeta.default_params — which its own docstring calls "persisted" — without raising: a set serializes in hash order, against the resume strict-match contract; a generator is consumed by the first call, so every later meta() persists []. Verified before changing anything: {"c","a","b"}['a','c','b'], and a generator gives ['a','b'] then [].

Why the call-time half had to follow. stop is echoed into request_params, which is persisted, so the bind-time fix on its own produced a rule that held at one entry point and not the other — ChatModel(stop={"a","b"}) raised while agenerate(stop={"a","b"}) was accepted and sent the set in hash order. Measured across four processes, the same five-element set produced a different order each time. tools / tool_choice are deliberately left taking any iterable: a set of tool definitions is impossible (mappings are unhashable) and the elements already go through _named_json_value.

Related Issues

Refs #25. Follow-ups from the review of #98, which flagged both as belonging in their own PR.

Test Plan

Automated

  • Lint/format clean (ruff check && ruff format --check)
  • Type check clean (ty check)
  • Unit tests pass (pdm run pytest) — 5332 passed. Three tests/performance gates (efficiency/memory ratios) fail only when the whole suite runs; all three pass in isolation on this branch. Load sensitivity, not a regression — the diff adds one isinstance check.
  • sieval/core/models/model.py line+branch coverage 99% (coverage run --source=sieval, per sieval/core/CLAUDE.md) — no uncovered line falls in the changed region.

Manual

  • Mutation-tested every new test — each reverted change fails only the tests that should catch it:

    mutation result
    usage reads the server-reported total again 2 failed
    _named_json_value widened back to Iterable 5 failed
    call-time stop widened back to Iterable 2 failed
    bind-time validation turned into a no-op 6 failed (both write sites)
  • Reachability scan — both narrowings close a trap, not a live defect. _kwargs has two writes (_initialize, with_args); builder_defaults comes only from the ChatModel/GenModel constructors. All eight construction sites are ChatModel(**config) from task YAML, which cannot express a set or generator. On the call-time side, every in-tree stop= passes a list(...) or a module-level tuple (human_eval, gsm8k, gsm1k, mbpp, livecodebench, openbookqa, hendrycks_math, theoremqa), and every other agenerate(...) argument is a scalar. The exposure is to out-of-tree task authors, whose symptom would have been a resume abort reporting a diff it cannot describe.

Checklist

Required (all PRs)

  • PR title follows conventional format (type(scope): description)
  • No internal paths, credentials, or personal info in committed files
  • AI-generated code has AI-Generated Code - <model> (<provider>) in module docstring — no new modules
  • No new upper-layer dependencies added to core/
  • Deleted code verified — the removed total-token check is unreachable once the total is computed; its test case goes with it

If: Breaking Change

  • Described what breaks and migration path in Summary
  • Existing tests updated to reflect new behavior

Programmatic callers only, and all of it is unreleased — there is no migration path to write, and no CHANGELOG entry. sieval/core/models/dialects/ and _named_json_value do not exist at v0.7.0, where Model.meta() was a bare dict(self._kwargs) with no JSON validation of default_params and self._kwargs = kwargs with no copy. Every behaviour below arrived in #45 after that tag, so no released version can have depended on it.

  • ChatModel(model=..., stop={"a","b"}) raises at construction instead of persisting hash-ordered output, and a non-finite default raises there rather than on the first meta().
  • agenerate(stop={"a","b"}) raises instead of sending the set in hash order.
  • The narrowing also reaches _named_json_value's other callers — tools[i], tool_choice, the structured-output schema, and dialect options — so a set or generator nested in any of those now raises where it used to be silently flattened into a list.

No in-repo caller is affected and YAML config cannot produce any of these. Two existing tests asserted the old timing, not the old outcome, and were updated accordingly.

@ethan-scitix ethan-scitix changed the title fix(models): compute usage totals and reject non-round-trippable default params fix(models): compute usage totals, and reject bad builder defaults at bind time Aug 13, 2026
@ethan-scitix
ethan-scitix force-pushed the refactor/dedup-model-primitives branch from 40007c3 to 529836d Compare August 13, 2026 17:34
@ethan-scitix
ethan-scitix force-pushed the fix/model-record-hardening branch from f50430c to a476b22 Compare August 13, 2026 17:34
Base automatically changed from refactor/dedup-model-primitives to main August 13, 2026 17:42
ethan-scitix and others added 4 commits August 14, 2026 01:43
… one

Token accounting had three stances across the codebase for the same concept:
openai_chat validated the reported total and raised when it did not decompose,
openai_completions accepted whatever the server sent, and the sglang transport
never read a reported total at all -- it summed the two counts. Both dialects
now agree with sglang.

The chat check was the risky one. OutputContractError is documented as "a
successful wire reply violated its declared output contract", and it is never
caught anywhere in sieval; it reaches the runner's generic except Exception,
consumes a retry, and lands the sample in fails scoring 0. So a server whose
total_tokens is off by a token discarded a reply whose tokens were already
generated and billed -- and in the streaming path the usage arrives on the
final chunk, so the rollout was thrown away only after completing.

Reading only the two counts we consume also makes the identity structural:
_validate_input_scoring_boundary's total-token comparison can no longer fire,
so it goes, replaced by a comment recording why. The remaining boundary checks
compare usage against the echoed logprob positions and still have teeth.

Both new tests were confirmed to fail when the production code is mutated back
to reading the server-reported total.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_named_json_value accepted any non-str/bytes Iterable, where the same-package
copy_json_value has always taken only list/tuple. Two of the shapes that gap
admits corrupt the persisted record without raising:

  * a set serializes in hash order, so ModelMeta.default_params differs run to
    run -- directly against the resume strict-match contract;
  * a generator is consumed by the first call, so every later meta() serializes
    it as [] and persists that. meta() runs once per response, so the record
    changes shape partway through a single run.

Narrowed to list | tuple. A scan of every write to _kwargs (two sites, fed by
the ChatModel/GenModel constructors and the derivation path) and every model
construction site (eight, all ChatModel(**config) from task YAML, which cannot
express a set or a generator) found nothing that reaches the branch today, so
this closes a trap rather than fixing a live defect.

All four new tests were confirmed to fail when the branch is widened back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… response

meta() runs once per response, so it was the first thing to notice a default
that cannot round-trip through JSON -- raising only after a model call had been
made and billed, and in a streamed call only after the rollout had finished.
That is the same shape this branch removes from usage accounting one commit
earlier, so it should not be reintroduced here.

Both writes to _kwargs now run the coercion: the bind path in _initialize and
the derivation path in with_args. Values are stored unconverted, since the
request builders need them as given and meta() does its own copy.

Two existing tests asserted the old timing rather than the old outcome, so they
now construct instead of calling meta(). All six tests covering the two write
sites were confirmed to fail when the check is turned into a no-op.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docstrings added by this branch argued their design at paragraph length.
The reasoning belongs in the commits and the PR; the code needs only the part
a reader cannot recover from the lines below it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ethan-scitix
ethan-scitix force-pushed the fix/model-record-hardening branch from a476b22 to a19a7e2 Compare August 13, 2026 17:47
Narrowing `_named_json_value` to list/tuple closed the bind-time door but
not the call-time one: `_kwargs_to_request` pops `stop` before that check
runs and handles it with its own `isinstance(stop, Iterable)` branch, so
`ChatModel(stop={"a","b"})` raised while `agenerate(stop={"a","b"})` was
accepted.

The value is echoed into `request_params`, which is persisted, so a set
landed on disk in hash order -- measured across four processes, the same
five-element set produced a different order each time. That is the
resume strict-match hazard the bind-time check exists to prevent, one
layer down.

Not a live defect: every in-tree `stop=` passes a list or a module-level
tuple, so this closes a trap for out-of-tree task authors, whose symptom
would have been a resume abort reporting a diff it cannot describe.

`tools` / `tool_choice` are deliberately left taking any iterable: a set
of tool definitions is impossible (mappings are unhashable) and the
elements already go through `_named_json_value`.

Tests: a set and a generator are refused on the same terms at both entry
points, and a tuple still reaches `SamplingParams.stop` -- bind time now
stores tuple defaults unconverted, so that path is load-bearing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ethan-scitix
ethan-scitix merged commit d0d2027 into main Aug 14, 2026
9 checks passed
@ethan-scitix
ethan-scitix deleted the fix/model-record-hardening branch August 14, 2026 02:01
ethan-scitix added a commit that referenced this pull request Aug 14, 2026
`model.py` held two things with different lifetimes: the canonical `Model`
composition root, which the plane keeps evolving, and the one-cycle
compatibility surface, which is scheduled to die. Kept in one file, removing
the compat layer is surgery on a live module; split out, it is a file deletion.

  model.py           1260 -> 664
  _legacy_bridge.py       476   task kwargs <-> Request/Response, ModelOutput
  _legacy_binding.py      170   RuntimeBindingPlan for the bare wrappers

The split holds because the bridge was already free-function shaped, as the
review measured: `_kwargs_to_request` (212 lines) touched only `self.dialect_id`
and the static `self._validate_n`, and `_response_to_model_output` only
`self.meta()`. Those three become parameters; nothing else changed.

Direction is now one-way, which is the property that makes the eventual delete
cheap: `_legacy_bridge` imports `deployment` and `ir` and nothing from `model`,
while `model` imports six names from it. `_legacy_binding` is reachable only
from `chat_model`/`gen_model`, so it goes when the wrappers go. Still zero
runtime import cycles across the 27 modules.

`uuid4()`'s non-deterministic binding fingerprint now sits alone in
`_legacy_binding`, where it is visible and removable, instead of buried
mid-file.

No behaviour change, and this is checked rather than asserted: every relocated
definition is byte-identical to its `main` source, or AST-identical once the
four renames this commit documents are applied (`_named_json_value`,
`_validate_n`, `_kwargs_to_request` and `_response_to_model_output` lose the
leading underscore on leaving the class), and the three self-to-parameter
conversions are AST-identical after the substitutions above.

That equivalence is measured against the current `main`, which matters because
#100 landed between the two and edited two of the definitions relocated here:
it narrowed `_named_json_value`'s sequence branch and `_kwargs_to_request`'s
`stop` branch to `list`/`tuple`. Both narrowings are carried into the new
homes, and the tests #100 added to guard them were confirmed to still fail when
either branch is widened back where it now lives -- three cases on the coercer,
two on `stop`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ethan-scitix added a commit that referenced this pull request Aug 14, 2026
Review of the split found `_legacy_bridge` had become the owner of
`named_json_value`, which `Model.meta()` needs too — so deleting the file
would have broken the canonical plane, the one thing the split exists to make
cheap. The primitive moves to `_shared`, beside `copy_json_value`, and both
callers import it from there. `_legacy_bridge` now holds only what dies with
it, and its inbound edge from `model` drops from six names to five.

The two coercers stay two functions, but the reason for it has narrowed. #100
brought `named_json_value` down to `list`/`tuple`, which is what
`copy_json_value` always took, so the only difference left is how an error
names a rejected leaf: one indexes into the list, the other stops at the
enclosing key. Folding them together still changes the messages callers see,
so it belongs in its own change rather than in a move. The module docstring
records what is left of the split, not the container disagreement that #100
removed.

`ModelMeta` is the one thread still to be cut by hand when the bridge goes:
`Model.meta()` is public and `tasks/ruler_0shot_gen.py` calls it outside the
legacy path. The module docstring says so now, instead of claiming the delete
costs nothing.

One deliberate behaviour change. `response_to_model_output` copies
`model_meta` before attaching provenance. As a method it built that mapping
itself, so mutating in place was safe by construction; as a free function it
receives one, and writing into it leaked the first response's provenance into
every later output built from the same mapping — into `ModelCallMeta`, which
is persisted. Both current call sites pass a fresh `meta()`, so nothing was
wrong on disk; the guarantee is restored before a caller can get it wrong. The
new test asserts the caller's mapping is untouched and that two outputs do not
share it, and fails without the copy.

`tests/unit/cli/test_resolution.py` resolved `ModelOutput` through
`sieval.core.models.model`, which only re-exports it — six paths that would
have broken at bridge-deletion time for reasons unrelated to the resolver.
They use `Model` now, which that module actually defines.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ethan-scitix added a commit that referenced this pull request Aug 14, 2026
* refactor(models): split model.py by lifetime, not by size

`model.py` held two things with different lifetimes: the canonical `Model`
composition root, which the plane keeps evolving, and the one-cycle
compatibility surface, which is scheduled to die. Kept in one file, removing
the compat layer is surgery on a live module; split out, it is a file deletion.

  model.py           1260 -> 664
  _legacy_bridge.py       476   task kwargs <-> Request/Response, ModelOutput
  _legacy_binding.py      170   RuntimeBindingPlan for the bare wrappers

The split holds because the bridge was already free-function shaped, as the
review measured: `_kwargs_to_request` (212 lines) touched only `self.dialect_id`
and the static `self._validate_n`, and `_response_to_model_output` only
`self.meta()`. Those three become parameters; nothing else changed.

Direction is now one-way, which is the property that makes the eventual delete
cheap: `_legacy_bridge` imports `deployment` and `ir` and nothing from `model`,
while `model` imports six names from it. `_legacy_binding` is reachable only
from `chat_model`/`gen_model`, so it goes when the wrappers go. Still zero
runtime import cycles across the 27 modules.

`uuid4()`'s non-deterministic binding fingerprint now sits alone in
`_legacy_binding`, where it is visible and removable, instead of buried
mid-file.

No behaviour change, and this is checked rather than asserted: every relocated
definition is byte-identical to its `main` source, or AST-identical once the
four renames this commit documents are applied (`_named_json_value`,
`_validate_n`, `_kwargs_to_request` and `_response_to_model_output` lose the
leading underscore on leaving the class), and the three self-to-parameter
conversions are AST-identical after the substitutions above.

That equivalence is measured against the current `main`, which matters because
#100 landed between the two and edited two of the definitions relocated here:
it narrowed `_named_json_value`'s sequence branch and `_kwargs_to_request`'s
`stop` branch to `list`/`tuple`. Both narrowings are carried into the new
homes, and the tests #100 added to guard them were confirmed to still fail when
either branch is widened back where it now lives -- three cases on the coercer,
two on `stop`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* refactor(models): keep the deletable bridge free of shared primitives

Review of the split found `_legacy_bridge` had become the owner of
`named_json_value`, which `Model.meta()` needs too — so deleting the file
would have broken the canonical plane, the one thing the split exists to make
cheap. The primitive moves to `_shared`, beside `copy_json_value`, and both
callers import it from there. `_legacy_bridge` now holds only what dies with
it, and its inbound edge from `model` drops from six names to five.

The two coercers stay two functions, but the reason for it has narrowed. #100
brought `named_json_value` down to `list`/`tuple`, which is what
`copy_json_value` always took, so the only difference left is how an error
names a rejected leaf: one indexes into the list, the other stops at the
enclosing key. Folding them together still changes the messages callers see,
so it belongs in its own change rather than in a move. The module docstring
records what is left of the split, not the container disagreement that #100
removed.

`ModelMeta` is the one thread still to be cut by hand when the bridge goes:
`Model.meta()` is public and `tasks/ruler_0shot_gen.py` calls it outside the
legacy path. The module docstring says so now, instead of claiming the delete
costs nothing.

One deliberate behaviour change. `response_to_model_output` copies
`model_meta` before attaching provenance. As a method it built that mapping
itself, so mutating in place was safe by construction; as a free function it
receives one, and writing into it leaked the first response's provenance into
every later output built from the same mapping — into `ModelCallMeta`, which
is persisted. Both current call sites pass a fresh `meta()`, so nothing was
wrong on disk; the guarantee is restored before a caller can get it wrong. The
new test asserts the caller's mapping is untouched and that two outputs do not
share it, and fails without the copy.

`tests/unit/cli/test_resolution.py` resolved `ModelOutput` through
`sieval.core.models.model`, which only re-exports it — six paths that would
have broken at bridge-deletion time for reasons unrelated to the resolver.
They use `Model` now, which that module actually defines.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ethan-scitix added a commit that referenced this pull request Aug 14, 2026
)

`UsageStats` declared `reasoning_tokens` and `cached_tokens` and no dialect
ever populated them, so a converted checkpoint that held its score while
burning twice the reasoning budget was invisible -- the delivery regression no
accuracy metric catches. Both OpenAI-shaped dialects now lift
`prompt_tokens_details` / `completion_tokens_details`, and sglang surfaces the
`cached_tokens` it was already reading twenty lines below for its radix-cache
guard.

The optional counts are `int | None`, not `int = 0`. Most OpenAI-compatible
servers omit the detail objects entirely, so a zero default makes "no cache
hits" and "never said" the same number, and any average over a mixed fleet
silently wrong. The distinction is carried to disk: `ModelUsage` gains
`NotRequired` keys written only where the server reported one, so an absent key
means unreported and a present `0` is a real measurement.

The same rule holds one layer up: `profile.json`'s `share` is absent when its
parent count is 0. A server counting reasoning outside its completion count can
report reasoning against 0 completion tokens, and `0.0` there is not a measured
0% but an undefined ratio -- the same lie the counts are optional to avoid.
`total` still carries what was reported.

No subset relation is enforced. `reasoning <= completion` is an OpenAI
convention rather than a wire guarantee, and a server counting reasoning
outside its completion count is exactly the one whose reported total exceeds
the computed one. That case is now recorded instead of discarded:
`reported_total_tokens` is kept only where it disagrees with prompt+completion,
so it is absent on almost every call and, when present, is the only evidence
that a provider's accounting differs from ours. #100 had nowhere to put that
difference and dropped it silently.

The profiler reports the new fields as shares against the tokens of the calls
that reported them -- never against every call, which understates a share by
the non-reporting fraction -- and prints that denominator on every line rather
than only when partial, since a coverage note given sometimes reads as full
coverage the rest of the time. They are never folded into `total_tokens`, which
stays prompt+completion.

`record_model_usage` and `aggregate_token_usage` carried the same accumulation
logic twice and now share one accumulator: a field added to the live path alone
would have vanished on the next resume. They share its admission test too --
the rebuild used to count a usage the live path skips, inflating `calls_total`
after a resume but not on the original run. The rebuild clears the new
accumulators for the same reason: they are additive.

The record projection lands in `_legacy_bridge`, not `model`. #99 moved
`ModelUsage` and `response_to_model_output` there after this work was branched,
and the conflict it raises is only reported against the old home.

Deliberately not collected: `audio_tokens`. Nothing in sieval sends or receives
audio, so it would ship as a permanently absent key.

Both changed record shapes are released -- `ModelUsage` and
`ProfileStageTokenUsage` each ship at v0.7.0 -- so these additions belong in the
release notes. Every new key is `NotRequired` and written only where the server
reported it, so nothing reading the existing keys breaks and no migration is
needed.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant