T043: llm-shared OpenAI-compat base class, retry helper, and OcrParams/LlmParams config gap fix#41
Conversation
…s/LlmParams gap fix - Add packages/iris-adapters/llm-shared/ with OpenAICompatProvider abstract base class and RetryConfig/with_retry exponential backoff helper. Internal workspace package; consumed only by llm-azure-openai, llm-openai, and llm-local. - Add OcrParams and LlmParams to AdaptersSchema in iris-config, closing a gap from WS003/WS004 where both were specified as "consumed from WS002" but never defined. All fields have safe defaults so existing product YAMLs remain valid. - Regenerate docs/schemas/product.schema.json to include the new param blocks. - Register iris_llm_shared in root_packages (pyproject.toml) and update workspace membership tests; exclude it from concrete-adapter contracts since it is a shared helper, not an independent adapter.
anmolg1997
left a comment
There was a problem hiding this comment.
Reviewed on head 39fa5ce. Ran the suite locally: 421 passed, lint and typecheck clean, coverage 96.45%. The base class is well-built and the proactive OcrParams/LlmParams gap fix is a good catch. Two concrete items before merge, both small.
1. C-LLM-008: retry count is not observable on the OTEL span (base-class gap)
The clause is explicit: "A mock provider returning 429 once and then 200 returns a valid LLMResponse after one retry. The retry count is observable via the OTEL span."
Right now with_retry() returns only the result and swallows the attempt count, and complete() sets five span attributes (model, input/output tokens, latency, structured_output_used) but never a retry count. So nothing carries the number of attempts onto the span.
This can only live in the base class, so no subclass in T044-T047 can satisfy the clause, and the contract suite in T048 will fail against it. T043's acceptance is 'a subclass passes the OpenAI-compatible contract clauses with zero per-subclass code', so this belongs here. Cheaper to wire now than to retrofit the base class once T048 is written.
Minimal shape: have with_retry track attempts and surface the count (return a small result tuple, or accept an on_attempt callback), then in complete() do span.set_attribute('llm.retry_count', n). Add a test: 429-then-200 succeeds and the span shows retry_count == 1 (the conftest span_exporter from T038 already gives you the captured spans).
2. iris_adapter_llm_shared missing from the coverage source list
[tool.coverage.run] source lists every other adapter (iris_ocr_adi, iris_ocr_datalab, ...) but not iris_adapter_llm_shared, so openai_compat.py and retry.py branches aren't gated by the 95 percent threshold even though test_unit.py exercises them well. Add iris_adapter_llm_shared to that source list to keep the gate consistent with every other package.
Minor nit (optional, your call)
OpenAICompatProvider decorates id/_base_url/_auth_headers with @AbstractMethod but the class isn't an ABC (no abc.ABC base or metaclass), so the decorator is decorative: a subclass that forgets _base_url() instantiates fine and fails later inside httpx with a confusing error rather than cleanly at construction. Making it inherit abc.ABC would give T044-T047 a clear failure at instantiation. Not blocking.
Everything else is solid: error-code mapping is correct and complete, the auth-no-leak path is tested, structured output is validated to LLMSchemaViolation, and the schema gap fix is non-breaking with all fields defaulted. Once the span retry count and the coverage source line are in, this is good to go.
…C base Address two required changes and one optional nit from Anmol's review on PR #41. - retry.py: with_retry now returns (result, retry_count) where retry_count is the number of retries performed (0 means first attempt succeeded) - openai_compat.py: unpack tuple from with_retry and set llm.retry_count on the OTEL span; inherit abc.ABC so missing abstract methods fail at instantiation - pyproject.toml: add iris_adapter_llm_shared to [tool.coverage.run] source - tests/test_unit.py: update with_retry tests to unpack and assert retry_count; add two span-attribute tests (retry_count 0 and 1) - tests/conftest.py: span_exporter fixture that attaches to the active TracerProvider
|
Hi @anmolg1997 - all three points addressed in ce197e0 (retry_count on span, iris_adapter_llm_shared in coverage source, ABC base). 423 tests passing. Ready for another look. |
anmolg1997
left a comment
There was a problem hiding this comment.
Re-reviewed on head 240f48a. All three items from the prior round are addressed, with tests, and the suite is green locally (423 passed, lint and typecheck clean, coverage 95.63% over the 95 gate).
- Retry count on the OTEL span (C-LLM-008) — done well.
with_retrynow returnstuple[T, int]with the attempt count,complete()setsllm.retry_count, and there are two new span tests: retry_count == 0 on the clean path and the exact C-LLM-008 scenario (429 then 200 gives retry_count == 1 on the span). That clause is now satisfiable by subclasses with zero per-subclass code. - Coverage source —
iris_adapter_llm_sharedis in the source list now, so the package is gated. Total dipped to 95.63% precisely because its lines are counted, which is the point. openai_compat.py sits at 89% on its own; the misses are the non-injected real-httpx client path and the rare non-400 4xx catch-all, which lines up with where the OCR adapters landed, so no concern. - ABC base (the optional nit) — taken:
class OpenAICompatProvider(ABC). T044-T047 now fail cleanly at construction if they miss a method.
The _run() event-loop close in a finally is a nice extra bit of test hygiene.
Good to merge. This base class is now the right foundation for the three OpenAI-compatible adapters stacked on top (#43, #44, #45).
Summary
Implements T043 - the internal
iris-llm-sharedworkspace package providing the shared HTTP base class and retry helper for the three OpenAI-compatible LLM adapters (llm-azure-openai,llm-openai,llm-local). Anthropic is standalone and does not inherit from this base. Also closes a config gap from WS003/WS004 whereOcrParamsandLlmParamswere never added toAdaptersSchema.packages/iris-adapters/llm-shared/src/iris_adapter_llm_shared/openai_compat.py:OpenAICompatProviderabstract base class. Subclasses override onlyid,_base_url(), and_auth_headers(). The base handles HTTP, error mapping (401/403/429/400/5xx to typed exceptions), structured output via JSON-mode, OTEL spans, and retry.packages/iris-adapters/llm-shared/src/iris_adapter_llm_shared/retry.py:RetryConfig+with_retry()exponential backoff, retrying only onLLMRateLimitedandLLMUnavailable.iris_config/schema/adapters.py:OcrParamsandLlmParamsadded toAdaptersSchema. Both were specified in WS003/WS004 plan.md files as "consumed from WS002" but WS002 only defined adapter name selection and never defined them. All fields useField(default_factory=...)so existing product YAMLs remain valid with no breaking change.docs/schemas/product.schema.json: regenerated to includeocr_paramsandllm_paramsblocks.pyproject.toml:iris_adapter_llm_sharedadded toroot_packages. Excluded from concrete-adapter contracts (layers, independence, forbidden) since it is a shared helper, not an independent adapter.iris_engine/llm/in_memory.py: hoisted function-local tracing import to module top, consistent with the pattern enforced in T038.Task reference
T043004-llm-adapter-setAcceptance criteria
T043
OpenAICompatProvidersubclass requires onlyid,_base_url(), and_auth_headers()- zero other per-subclass codeLLMRateLimitedandLLMUnavailable, not on other errorsLLMSchemaViolationon mismatchcontent_filterfinish reason raisesLLMContentFilteredlengthfinish reason raisesLLMContextWindowExceededPR checklist
tasks.mdentry is satisfied.docs-ciworkflow passes (markdown lint + tasks structural check).Notes for the reviewer
OcrParams/LlmParams gap flagged explicitly: Both params blocks were in WS003/WS004 plan.md files as "consumed from WS002" but WS002 only defined enum-backed adapter name selection with no mention of params. T043 closes both gaps. Defaults mean no product YAML requires changes.
Module named
iris_adapter_llm_shared: Following the naming convention from the WS004 plan (iris_adapter_llm_*). Concrete adapter scaffolds (iris_llm_*) will be renamed as each is implemented in T044-T047.iris_adapter_llm_sharedis not a concrete adapter: Excluded from the independence and forbidden contracts deliberately. It appears inroot_packagesonly so import-linter is aware of it.