Skip to content

feat(models): capability-based Model IR + Transport frontends (RFC #25) - #45

Open
jack-scitix-ai wants to merge 2 commits into
scitix:mainfrom
jack-scitix-ai:ir
Open

feat(models): capability-based Model IR + Transport frontends (RFC #25)#45
jack-scitix-ai wants to merge 2 commits into
scitix:mainfrom
jack-scitix-ai:ir

Conversation

@jack-scitix-ai

Copy link
Copy Markdown
Contributor

Type

  • feature — new benchmark, task, or capability

Summary

  • Implement the RFC [RFC]: A capability-based Model IR — decouple model access from the OpenAI protocol #25 endstate: arun(Request) -> Response is the one primitive (acquires limiters, delegates to a composed Transport); agenerate/alogprobs become thin, capability-gated wrappers over it.
  • Each provider wire protocol is now a Transport frontend (lower/lift). All wire logic (streaming accumulation, echo split, logprob parsing) moved out of the three Model backends into transports/ (openai_chat, openai_completions, sglang); the backends are thin transport selectors.
  • Capability catalog + assert_capability: a Request using an unsupported feature is rejected at setup. alogprobs(echo=True) on a chat backend now raises CapabilityError instead of being silently ignored (historical bug).
  • Task.requires declares needed capabilities, asserted at construction (capability-based replacement for the type: chat|gen isinstance path).
  • Also fixes the checked-in [tool.mutmut] config (never ran in a clean env): also_copy was missing package roots + scripts/; added mutation-only collection skips for live-repo / fresh-interpreter tests.

Related Issues

Refs #25, Refs #24

Test Plan

Automated

  • Lint/format clean (ruff check && ruff format --check)
  • Type check clean (ty check or mypy --strict)
  • Unit tests pass (pdm run pytest)

Manual

  • Full gated suite: pytest tests/unit tests/integration --cov --cov-fail-under=95 → 2496 passed, coverage 98.29%
  • Mutation testing (core/CLAUDE.md ≥70% gate) on changed modules: 1500 mutants, aggregate 89.6%, every module ≥70% (min: sglang transport 80.6%)
  • Backward-compat regression suite (tests/integration/test_model_backward_compat.py): legacy ModelOutput surface unchanged; echo-on-chat now raises CapabilityError

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 upper-layer dependencies added to core/
  • Deleted code verified — no remaining call sites depend on it

If: Breaking Change

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

@ethan-scitix ethan-scitix left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict

The wire extraction is correct — I verified it. The skeleton it lands on needs rework before merge.

Splitting those two apart, because the second conclusion should not be read as doubt about the first.

What I verified empirically

  • pdm run pytest tests/unit tests/integration2509 passed; ty check sieval clean; ruff clean apart from the pre-existing generated sieval/_version.py.

  • Coverage on changed modules: ir.py/sglang.py/capabilities.py 100%, both OpenAI transports 98%, model.py 95% — meets the core/CLAUDE.md gate. (I did not re-run mutmut.)

  • A/B'd the legacy backends against the new transports on identical wire payloads. I extracted origin/main:sglang_gen_model.py and origin/main:gen_model.py and ran both implementations over the same realistic sglang /generate and vLLM /v1/completions echo responses:

    sglang / vLLM × echo on/off — four paths:
      texts / finish_reasons / logprobs_tokens / logprobs / usage  →  bit-identical
    

    The PPL/CLP-critical fields survive the rewrite exactly. The only divergence is top_logprobs under echo=True (legacy returned prompt+completion positions, new returns completion-only) — no in-tree task reads it, every CLP task uses echo=False and every PPL task uses logprobs=0, and the new behaviour actually matches what choice_scores_from_top_logprobs documents ("top_logprobs[0] (the next-token distribution)"), which the legacy echo layout violated. That's a latent-bug fix, and it's annotated at sieval/core/models/model.py:509-513. It should be in the PR summary too, since it is user-visible for anyone building on alogprobs(echo=True, logprobs>0).

So the streaming accumulation, echo split, sglang triple parsing, token-text normalisation and radix guard are all faithful transpositions. That's the expensive part, and it's done.


Why the skeleton needs rework

The capability abstraction was built, but neither decision point was migrated onto it.

Task.model_type + isinstance (sieval/core/tasks/task.py:79) and the config-layer type: binary are both untouched, while Task.requires (task.py:60) has zero users in the entire repo. So the PR ships two parallel systems where the new one decides nothing. Everything below is a symptom of that, not an independent problem:

1. The IR silently ignores four Request features — the exact failure mode this PR exists to remove. transports/openai_completions.py:92 and transports/sglang.py:159 read neither req.session_id, req.server_tools, req.suffix, nor any req.reasoning axis. openai_chat.py:110 does reject session_id, so siblings disagree on one field. Verified:

Request(input="p", session_id="resp_abc", server_tools=(ServerToolSpec(type="web_search"),),
        suffix="TAIL", reasoning=ReasoningParams(effort="high", budget_tokens=4096))

completions wire body: {'model': 'm', 'prompt': 'p', 'stream': False}
sglang wire body:      {'text': 'p', 'sampling_params': {}}
chat:                  CapabilityError: ...does not support stateful session_id

assert_capability only fires where a caller remembers to call it, and nothing calls it for these.

2. The capability catalog contradicts the implementation, and rejects features that work. OpenAIChatTransport lowers reasoning_effort (openai_chat.py:156) and lifts reasoning text, but declares neither Reasoning nor ReasoningEffort:

reasoning_effort actually sent on the wire: True high
reasoning actually lifted: ReasoningOutput(text='deep', ...)
assert_capability(Reasoning)       -> REJECTED: OpenAIChatTransport does not support: Reasoning
assert_capability(ReasoningEffort) -> REJECTED: OpenAIChatTransport does not support: ReasoningEffort

That's fail-loud in the wrong direction. 15 of the 24 Capability members are declared by no transport at all and are unreachable from any Request field any transport consults.

3. Capability is a Flag but capabilities are stored in a frozenset — a composite is always reported missing. capabilities.py:13 + model.py:237. Verified false positive on two supported caps:

c.assert_capability(Capability.Chat | Capability.FunctionCalling)
# both individually supported: True True
# CapabilityError: OpenAIChatTransport does not support: Chat|FunctionCalling

Declaring Flag invites |, and Task.requires: ClassVar[frozenset[Capability]] makes frozenset({Capability.Chat | Capability.SampledLogprobs}) a natural authoring mistake that silently rejects a working model. Either a plain Enum, or store a single bitmask and test caps & required == required.

4. Removing cross-kind derivation dropped quota sharing with no replacement, and left a footgun. as_type shared one client and one limiter across kinds; the docs now say "define a separate base model instead" (docs/guide/configuration.md:31), which means a second AsyncOpenAI client and a separate pool — losing the hierarchical concurrency core/CLAUDE.md treats as an engine invariant. Model.__init__ already takes transport= (model.py:133), which is the seam that would preserve sharing, but with_args doesn't handle it:

d = base.with_args(transport=OpenAICompletionsTransport(...))
# derived _transport is base transport? True
# extra_wire_params: {'transport': 'OpenAICompletionsTransport'}   → forwarded to the API

Please don't remove a feature before its replacement exists — as_type can stay until the revision provides a transport-swap that keeps the pool.

5. openai_completions.py:186 — when the server omits usage, echoed prompt tokens are labelled as sampled output. boundary = usage.input_tokens if usage is not None else 0. Verified on a vLLM-shaped echo response with usage=None:

input_scoring.token_logprobs: ()
logprobs (claimed SAMPLED output): 'Q', ' is', ' it', '?\n', ' A', ' B'   # 5 are prompt

No shipped task is affected (the legacy bridge re-concatenates), but arun/Response is the documented forward path and a new consumer gets prompt tokens as output with no signal. Contrast the sibling: SglangTransport._guard_radix_cache raises when it can't verify the echoed length rather than scoring silently. Same stance here — refuse the split when the boundary is unknowable.

6. Two different "what kind is this model?" implementations in one PR. session.py:1062 uses Capability.Chat in base_model.capabilities; task.py:79 uses isinstance. Pick one.


Two claims in the code that don't hold

Worth correcting before the revision builds on them:

  • top_k is not a "vLLM extension upstream OpenAI rejects" that warrants a first-class field (openai_chat.py:131, openai_completions.py:108). leaderboards/qwen3_proxy_5min_202606.yaml:78 already passes it through extra_body. So promoting top_k_sampling to SamplingParams wasn't required — the repo's own configs route it through passthrough. It's a good illustration of the promotion rule the revision needs.
  • "sglang's /v1/completions rejects echo=True together with logprobs" (sglang_gen_model.py:3, transports/sglang.py:3) has no supporting record anywhere in docs/designs/ — I searched. It is the sole stated justification for SglangGenModel existing. Meanwhile docs/superpowers/specs/2026-07-01-recipe-capability-layer-design.md documents a different constraint under a "Verified backend behavior" heading: input logprobs × prefix cache, on both engines (sglang truncates silently, vLLM V1 errors). The rejection claim looks like a possible mis-attribution of that to the protocol layer. That same section also states vLLM V1 has prompt_logprobs, which makes openai_completions.py:6's "missing native scoring endpoint" wrong and the whole echo workaround (plus its boundary split) potentially unnecessary.

Two probes gate the revision's scope, because each can delete an entire code path. Please run these before reworking:

  1. Does sglang's /v1/completions actually accept echo=True + logprobs? If yes, the native /generate frontend may be removable — along with token-text normalisation, --skip-tokenizer-init handling and the max_new_tokens=0 clamp.
  2. Is prompt_logprobs available on the deployed vLLM? If yes, the echo workaround and the usage.input_tokens split both go away.

Direction for the RFC #25 revision

The IR concept is right — input declares capability, the frontend translates to each vendor's fields. What went wrong is the population order: the vocabulary was filled from vendor docs instead of growing from frontends that exist. So the revision keeps the idea and changes how a field earns its way in.

  1. Split frontend from engine. Capability is f(frontend, engine, instance). vLLM and sglang both serve openai_completions; sglang also serves native /generate — so SglangGenModel fusing "sglang engine" + "native protocol" into one class is the root of several oddities here, including the radix guard protecting only one of the two sglang paths. This is the one thing that cannot be deferred: it determines the class structure, and #47's engine-level constraints have nowhere to live without it.

  2. Two-level capability with domains. First level = a concern that ≥2 providers express differently (sampling, scoring, reasoning, tools, structured_output, caching, session, modality). Second level = the fields inside it, each carrying a domain, not just a boolean — effort is a different enum per vendor, and top-k breadth is "supported up to N", not "supported". First-level groups should map 1:1 onto Request sub-records so there is one structure, not two.

  3. Derive the gate structurally, drop the central catalog. Have each frontend declare which Request fields its lower() consumes, and assert in Model.arun that no field set to a non-default value went unconsumed. Request is a frozen dataclass with defaults, so this is computable — a ~10-line prototype catches all four silently-dropped fields across all three transports with no Capability enum involved. Then a missing or wrong enum member can never cause a silent drop, and the field-promotion rule becomes enforceable: promote only when ≥2 frontends lower it to different wire names (translation coupling), otherwise leave it in extra_wire_params. That's CLAUDE.md's "extract on coupling, not on call count".

  4. Keep the output side closed — do not make it polymorphic. Response carries @sieval_record and is the persisted schema coupled to RFC #24's resume gate; an open subclass hierarchy makes the on-disk shape depend on which subclass showed up, which cannot be versioned. Instead: one optional field per modality, each typed as a record (the existing InputScoringResult / ReasoningOutput / UsageStats pattern), additive only, never renamed or retyped. That's also how the currently-missing channels land — embedding, media — without polymorphism.

  5. ChatModel / GenModel become deprecated aliases for the OpenAI chat-completions and completions bindings. After this PR they're 26 and 22 lines that only pick a transport, which Model(transport=...) already does; the isinstance kind check is the only thing keeping them alive. Note the sequencing constraint: if they become factory/partial aliases, isinstance(m, ChatModel) raises TypeError: isinstance() arg 2 must be a type — loudly, not silently — so the alias change and the requires-replaces-model_type migration must land together. Also drop the Model[TModelInput] generic and the openai.types.chat.ChatCompletionMessageParam leak: a provider-agnostic layer shouldn't pin its Model signature to one vendor's SDK type, and it currently disagrees with Request.input's own str | list[dict[str, Any]].

  6. Naming. Transport collides with httpx (tests/unit/scripts/test_check_preflight.py:1025 literally has transport=transport), and backend is already triple-booked — sieval/infer/backends/ (launch-side translators), and this PR's own docstrings (core/models/__init__.py:1 "Model backends", chat_model.py "the backend selector"). Suggest Frontend: the PR's docstrings already use "provider frontend" 10 times in prose and it has no collision. That frees backend to mean the engine only.

  7. Reject serving-axis capabilities in requires rather than accepting and ignoring them. scoring.topk_breadth and caching.prefix_cache are enforced by #47, not this revision. Better that nobody can write the declaration than that someone writes one which silently does nothing — that's the same failure as items 1–2 above, and it would be ours.


Also

  • Config-layer type: chat|gen should stay as-is in the revision. It's hard-coded in 6 places (task.py:58, tasks/meta.py:73,143, cli/validation.py:151, session.py:64,1056) and feeds sieval/meta/index.json (35 model_type occurrences, schema_version: 1), so changing it triggers the meta-drift preflight and wants its own RFC. Just say explicitly in the revision that it's known-remaining — otherwise whoever adds the Anthropic frontend will assume the groundwork is done.
  • docs/superpowers/specs/2026-07-01-recipe-capability-layer-design.md is not tracked by git. It's the only record of the verified backend behaviours, it's explicitly related to #21/#24/#25, and it designs the launch-flag half of the same vocabulary. Please commit it — it should probably be merged into the RFC #25 revision.
  • Two small ones: _SAMPLING_PARAM_MAP in sglang.py:47 is now mostly dead (7 of 9 keys are first-class SamplingParams fields popped before they can reach it, so only min_p and repetition_penalty are reachable); and sglang.py:186 loses the legacy max(max_tokens, 1) clamp on the echo=False path, so alogprobs(echo=False, max_tokens=0) now forwards max_new_tokens=0, which sglang rejects.
  • The bundled [tool.mutmut] fix and the two collect_ignore guards are fine as-is — disclosed with clear rationale. I confirmed also_copy = [..., "scripts"] is not redundant with the tests/unit/scripts skip, because tests/unit/core/tasks/test_meta_pilot.py also reaches into scripts/.
  • Title should be feat(models)!: — this removes public API and invalidates previously-working configs, and CHANGELOG.md is generated from commits at release time.

Filed alongside this review

  • #47 — RFC for the capability constraint layer (the cross-group constraints this PR's Capability set cannot express: input_logprobs ⊥ prefix_cache, top-k breadth floors, protocol-mandatory fields). Depends on items 1, 2 and a verify_instance() seam above.
  • #46fix(scripts): enforce relative-import scope in check_layer_imports. CLAUDE.md ## Import Policy and CONTRIBUTING.md:59 both say "same package: relative; cross-package: absolute", but nothing enforced the second half. The 7 from ..capabilities / from ..ir / from ..exceptions imports in transports/*.py are currently the only .. imports in the tree and every other nested subpackage uses the absolute form (task.py:12, session.py:27). Once #46 lands, python scripts/check_layer_imports.py will flag them with the absolute form to use — worth rebasing onto it and converting them either way.

Happy to review the RFC #25 revision before implementation — the class-structure decision in item 1 is the one worth agreeing on first, since everything else follows from it.

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.

2 participants