[Backend] Pass LLMConfig through the generation paths instead of destructuring it into loose parameters #484
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found while implementing #351, and deliberately not fixed there — see the discussion on PR #483.
The pattern
Every generation path takes the LLM target as four separate parameters —
endpoint_url,api_key,model,context_tokens— destructured fromLLMConfigat the boundary and re-threaded by hand through every helper. As of#351 it is five, because
llm_providerhad to travel the same way.Concretely, adding one field to
LLMConfigcost:llm_provider=llm_cfg.provider)That is ~58 edits and four rounds of test-double breakage for a field that
already existed on an object every one of those call sites started from.
Why it matters beyond the churn
The repo already knows this does not hold. From
DEFAULT_CONTEXT_TOKENS(llm_service.py:79-82):
That comment was written about
context_tokensand it is now equally true ofllm_provider. The mitigation both times was to make the absence benign — aconservative default window, a fallback to URL sniffing — which works, but it
means the safety comes from the default rather than from the structure, and the
next field may not have a benign default.
context_tokensis the cautionary case: its silent-absence bug (#337) shippedbecause seventeen call sites did not opt in, and the fix was to make the
parameter impossible to omit meaningfully, not impossible to omit.
Proposed fix
Pass the config object (or a small frozen
LlmTarget) instead of its fields.A new call site then cannot forget a field of an object it is already holding,
and adding a sixth attribute is one edit rather than fifty-eight.
Rough shape:
LlmTarget(endpoint_url, api_key, model, context_tokens, provider)— or justreuse
LLMConfig, which is already exactly this.generate_structured_text(prompt, target, *, system_prompt, ...), keeping theloose-parameter form as a thin deprecated shim during migration so the change
can land in reviewable pieces rather than one 6-file commit.
audio_service,beat_service,lore_service,generation_service,reminder_tasksandplanning_taskstaketargetandpass it on unchanged.
Scope notes
to do and easy to verify: the existing suite should pass untouched apart from
the doubles.
LLMConfiginstances rather thanSimpleNamespace, which is what let them drift from the dataclass in thefirst place (#441's concern — a double that diverges is a test that passes
while production breaks).
_qa_*functions inaudio_servicebuild their own request bodiesand bypass the transport entirely (#426). They should be folded in at the same
time, or the count goes back up.
Milestone
Filed under v4.6.0 with the rest of the simplification work (#443), but it
gets cheaper the sooner it happens — v4.2.0 and v4.3.0 will both add
generation call sites, and each one is another hand-threaded copy. Worth pulling
forward if there is an appetite for it before then.
Picking this up now, ahead of the rest of v4.2.0, and moving it from v4.6.0 to v4.2.0. Reasoning: #356, #357 and #358 each need the provider object at the generation call sites, so every one of them done before this adds another hand-threaded parameter through the same signatures. Doing this first makes each of those a one-object change.
Measured on
mainat233dc71(post-#483)endpoint_url: stracrossaudio_service,beat_service,generation_service,lore_service,reminder_tasks,planning_tasksgenerate_structured_textcall sitesllm_provider=occurrencesgenerate_structured_text_qa_*this issue names and the four_summarise_*get_llm_config/require_llm_configApproach
LLMConfigas the target. It is already exactly the five fields; a second type would be one more thing to keep in sync.generate_structured_text(prompt, target, *, ...)with the loose form removed outright, not shimmed. The point of this issue is that a new call site cannot omit a field of an object it is holding; a deprecated shim keeps the omit-able path alive for exactly the "eighteenth call site written next month" theDEFAULT_CONTEXT_TOKENScomment warns about. All call sites are in this repo, so there is nothing for a shim to protect.target: LLMConfigand pass it on unchanged. Origin sites stop destructuring.targettoo, so the count cannot climb back up. Merging them into one transport is out of scope here — that is the simplification pass (#443) and it changes behaviour surface; this issue does not.LLMConfiginstances.Will land as one PR, committed in reviewable pieces (transport →
audio_service→ other services → tasks/routers → tests).claude-bot referenced this issue2026-09-05 00:17:15 +00:00
Landed in PR #489 (merged 2026-09-05, CI green). Nine commits, 45 files, 1716 tests passing.
What was done
LLMConfignow lives inapp/providers/llm.py, frozen, next toLlmCapabilities/LlmProvider, withDEFAULT_CONTEXT_TOKENSand its rationale block. It had to move:settings_serviceimports fromllm_service, so the transport could not take the config from settings without a cycle.settings_servicere-exports the name.registry.llm_provider_for(target)is the one-object resolve;resolve_llm_providerstays for the two callers that genuinely hold the fields loose.generate_structured_text(prompt, target, *, ...)— the loose quintet is gone with no shim, and aninspect.signatureguard test keeps it gone. All four_structured_*, four_summarise_*, four_qa_*and four_test_llm_*transports taketargettoo, so the destructuring did not just move one frame down.audio_serviceplusbeat_service,generation_service,lore_service,reminder_tasks,planning_tasksand two routers converted; origin sites passllm_cfgwhole. The eight Whisper ASR functions were deliberately left alone (#350).evals/LiveProviderwas the one caller outsideapp/and had no test cover; it now carries anLLMConfigand gainedQB_EVAL_PROVIDER.LLMConfiginstances everywhere; noSimpleNamespacelook-alikes remain.endpoint_url: strsignatures inapp/llm_provider=inapp/Verified no behaviour change
An AST walk over the pre-change tree found zero production call sites reaching the transport with
context_tokensomitted (45 in tests, all migrated), so no production path's effective window moved. The three places the window is now read offprovider.capabilities.context_tokensare identical to the oldresolve_window(...)for any positive int, andget_llm_configalways resolves one.Found on the way
#488 —
_dispatch_proseandtest_llmaccepted the explicit provider and ignored it. Fixed in the same PR as its own commit; see that issue.Left open, on purpose
LLMConfigbeing frozen means a variant is made withdataclasses.replace;evals/providers.py:with_windowis the first user.