refactor(models): split model.py by lifetime, not by size - #99
Merged
Conversation
Collaborator
Author
|
ethan-scitix
force-pushed
the
refactor/dedup-model-primitives
branch
from
August 13, 2026 17:34
40007c3 to
529836d
Compare
ethan-scitix
force-pushed
the
refactor/split-model-legacy-bridge
branch
from
August 14, 2026 03:29
e919f6e to
d2234d4
Compare
`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>
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
force-pushed
the
refactor/split-model-legacy-bridge
branch
from
August 14, 2026 04:07
d2234d4 to
a9ce8ef
Compare
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>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Type
refactor — code restructuring, no behavior change
One scoped exception, called out under Manual:
response_to_model_outputnow copies the caller'smodel_metabefore attaching provenance. Both current call sites pass a freshmeta(), so nothing on disk was ever wrong — the guarantee is restored before a caller can get it wrong.Summary
model.pyheld two things with different lifetimes: the canonicalModelcomposition 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.pyModel_legacy_bridge.pyRequest/Response, theModelOutputfamily_legacy_binding.pyRuntimeBindingPlanfabricated for the bareChatModel/GenModelconstructorsThe split works because the bridge was already free-function shaped, as the review measured:
_kwargs_to_request(212 lines) touched onlyself.dialect_idand the staticself._validate_n;_response_to_model_outputonlyself.meta(). Those three become parameters. Nothing else changed.Direction is now one-way, which is the property that makes the eventual delete cheap:
_legacy_bridgeimportsdeployment+irand nothing frommodel._legacy_bindingis reachable only fromchat_model/gen_model, so it leaves when the wrappers do.uuid4()'s non-deterministic binding fingerprint (prior review N1(a)) now sits alone in_legacy_binding, visible and removable, instead of buried mid-file. This PR does not change it.Second commit applies this PR's own review findings. The split had made
_legacy_bridgethe owner ofnamed_json_value, whichModel.meta()needs too — so deleting the bridge would have broken the canonical plane, the one thing the split exists to make cheap. The primitive moves to_sharedbesidecopy_json_value, andmodel's inbound edge on the bridge drops from six names to five.ModelMetais the one thread still to be cut by hand at deletion time (Model.meta()is public andtasks/ruler_0shot_gen.pycalls it outside the legacy path); the module docstring says so now instead of implying the delete is free.Related Issues
Refs #25 — follow-up from the PR #45 review (N6). Does not close the
core/binding/split, which is still gated on step 5 moving SGLang and on settling whethercapabilities.pyis vocabulary or binder logic.Test Plan
Automated
ruff check && ruff format --check) — 495 files formattedty check)pdm run pytest) — 5336 collected, all green, 5 deselected (unit + integration);tests/unit/core/models/alone is 815; preflight 25/25, no FAIL/WARN. One repeat run flaked on the two load-sensitive performance tests (test_benchmark_scenariosat 55.5% concurrency efficiency,test_pipeline_memory_scalingat 6.6×) while the box was busy; both re-ran green in isolation and neither importscore/models.Manual
No behaviour change, checked rather than asserted. Every moved definition was compared against its post-fix(models): compute usage totals, and reject bad builder defaults at bind time #100
origin/mainsource (d0d2027f), not against the pre-fix(models): compute usage totals, and reject bad builder defaults at bind time #100 tree. Identity is measured on the first commit, the one that does the moving; the second commit's single deliberate change is listed separately in the last row of this section.ModelUsage,ModelMeta,ModelCallMeta,ModelOutput,_optional_float,_optional_int,_pop_compatible_alias,_LegacyOpenAIBinding,_legacy_runtime_plan,build_legacy_openai_binding_named_json_value→named_json_value,_coerce_structured_outputvalidate_n,kwargs_to_request,response_to_model_outputself→ parameter substitutions (73 top-level body statements: 4 + 55 + 14)_checked_builder_defaults(new in fix(models): compute usage totals, and reject bad builder defaults at bind time #100, stays inmodel.py)fix(models): compute usage totals, and reject bad builder defaults at bind time #100's work is carried, not silently reverted. fix(models): compute usage totals, and reject bad builder defaults at bind time #100 narrowed
_named_json_value's sequence branch and_kwargs_to_request'sstopbranch tolist/tuple. Because the rebase relocated both definitions, git had no conflict to mark at their new locations — so this was checked by reverse-mutation: widening each branch back where it now lives makes fix(models): compute usage totals, and reject bad builder defaults at bind time #100's own guards fail (3 cases on the coercer, 2 onstop). fix(models): compute usage totals, and reject bad builder defaults at bind time #100's five test names are all present and the models suite went 73 → 74 tests.Zero runtime import cycles across all 27 modules in
core/models(TYPE_CHECKING excluded), same as before._legacy_bridgeout-edges:_shared,deployment,ir.The relocated bridge is live code, not an unreferenced copy — three mutations, all caught by the suite:
validate_nacceptingn < 1,kwargs_to_requestdropping the binding-resource guard, andresponse_to_model_outputdropping the provenance attachment (that last one is caught bytests/unit/core/runners/test_runner.py, outside the model tests).The one deliberate behaviour change is guarded. As a method,
_response_to_model_outputbuilt themodel_metamapping 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 — intoModelCallMeta, which is persisted. The new test asserts the caller's mapping is untouched and that two outputs do not share it, and was confirmed to fail without the.copy().Checklist
Required (all PRs)
type(scope): description)AI-Generated Code - <model> (<provider>)in module docstring — both new modules carry itcore/sieval.core.models.modeland now use the publicsieval.core.modelspath; one test reached for the private_response_to_model_outputand now calls the free function.tests/unit/cli/test_resolution.pyresolvedModelOutputthroughsieval.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 useModelnow, which that module actually defines.Follow-up (not in this PR)
copy_json_valueandnamed_json_valueare still two functions. After fix(models): compute usage totals, and reject bad builder defaults at bind time #100 they no longer disagree about containers — both takelist/tuple— 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 changes the messages callers see, so it belongs in its own change rather than in a move.