Provider adapters + telemetry ownership (issue #16, steps 2–3) - #19
Merged
Merged
Conversation
Replace the 812-line LLMHandler monolith -- which repeated the same provider dispatch four times and built clients from module globals -- with one ProviderAdapter per provider behind a Protocol seam. - OpenAICompatibleAdapter (OpenAI + GLM), AnthropicAdapter, OllamaAdapter, DummyAdapter; each constructed with an injected client factory so every provider is unit-testable with a fake. Anthropic and Ollama gain their first real coverage. - LLMHandler selects one adapter at construction; the four dispatch tables collapse to that selection. Public interface preserved; prompt variants translate to the messages form. - generate() returns GenerationResult(text, usage); usage is provider-reported where available, adapter-counted as fallback. Streaming yields text then a terminal Usage -- LLMHandler drops it for now to keep the backend untouched; step 3 surfaces it to telemetry. - Behavior preserved: only ProviderUnavailableError (missing SDK, missing GLM key, Ollama connection refused) triggers the dummy fallback; real API errors propagate. Establishes docs/adr/ (ADR 0002) and CONTEXT.md glossary conventions. Sequencing step 2 of #16. 299 tests pass (279 baseline + 20 new).
Fix the inverted dependency and the reconstructed-prompt cost estimate in one step. - Move count_tokens and pricing (cost_usd/MODEL_PRICES/ModelPrice) from src/eval/ to a core src/telemetry/ package (tokens.py, pricing.py). The eval package now imports from core, never the reverse; src/eval re-exports the pricing symbols so its public API is unchanged. Old eval modules deleted (git mv, not shimmed). - Backend consumes provider-reported usage: the sync path calls generate_with_usage (real prompt/completion tokens); the streaming path reads the Usage carried on the stream's terminal event. LLMHandler's stream_response/stream_messages now yield str | Usage; the orphaned generate_with_context is removed. The reconstructed prompt strings and their count_tokens calls are gone. - Behavior preserved: telemetry still covers the answer pass only (the reasoning pass's usage is discarded); the no-documents early return still emits zero telemetry; StageTelemetry's shape is unchanged, so the API and frontend are untouched. test_backend_telemetry.py passes unchanged. ADR 0003; CONTEXT.md gains the telemetry term. Sequencing step 3 of #16. 300 tests pass.
- Split src/llm_handler/__init__.py (was 323 lines, over the 250 ceiling) into the LLMHandler facade + a providers module owning provider detection, client-factory construction, and model listing. - Modernize type syntax (list / str | None) and drop the three `# type: ignore` on the optional-import guards (typed as ModuleType | None). - Add missing docstring summary lines to the adapter/handler constructors; tighten the `-> dict` request helpers to dict[str, object]; hoist base.counted_usage's count_tokens import to module level. - Add first coverage for the GLM provider-selection branch (detect_provider, build_adapter returns the OpenAI-compatible adapter, and the GLM_API_KEY-missing dummy fallback). 303 tests pass.
The streaming telemetry tests only drove stream_query without a conversation_id, leaving the multi-turn branch — which captures the answer stream's terminal Usage and persists the assistant message — unexercised. Add a test that drives it with a real conversation_id and asserts telemetry, the done event's ids, and message persistence. 304 tests pass.
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.
Implements sequencing steps 2 and 3 of the RAG architecture deepening spec (#16). Step 1 landed in #17; steps 4–6 (Retriever/QueryEngine, backend split, WebSocket adapter, typed API seam) remain and are out of scope here.
Step 2 — Provider adapters with injected clients (ADR 0002)
Replaces the 812-line
LLMHandlermonolith — which repeated the same provider dispatch four times and built clients from module globals — with oneProviderAdapterper provider behind aProtocolseam:OpenAICompatibleAdapter(OpenAI and GLM),AnthropicAdapter(owns the system-message split),OllamaAdapter(owns the/api/chatpayload),DummyAdapter.LLMHandlerselects one adapter at construction (four dispatch tables → one selection). Public interface preserved; prompt variants translate to the messages form.GenerationResult(text, usage); usage is provider-reported where available, adapter-counted as fallback. Streaming yields text then a terminalUsage.ProviderUnavailableError(missing SDK / GLM key / Ollama connection refused) triggers the dummy fallback; real API errors propagate.Step 3 — Telemetry ownership (ADR 0003)
src/eval/to a coresrc/telemetry/package. The eval package now imports from core, never the reverse (verified: nosrc.evaltelemetry import remains in production).generate_with_usage, the streaming path via the terminalUsage— deleting the reconstructed-prompt token counting at all four assembly sites. The orphanedgenerate_with_contextis removed.StageTelemetry's shape is unchanged — the API and frontend are untouched.Deliberate behavior change
Consolidating the two dummy fallback variants means a single-prompt
generate()on the no-provider path now returns the messages-based placeholder ("…Your last message was N characters (conversation has N turn(s))…") rather than the old"…Your prompt was N characters…". Only visible when no API keys are configured; the[LLM unavailable]marker is unchanged. Recorded here as a decision, not an accident.Verification
CI_LLM_MOCK=1 python -m pytest tests/ -v→ 304 passed (279 baseline + 25 new).test_backend_telemetry.pypasses unchanged (behavior-preservation proof), plus new coverage for the streaming conversation branch.Any/# type: ignoreadded.docs/adr/+CONTEXT.mdglossary conventions;Architecture.mdupdated.Closes part of #16 (steps 2–3).