UN-4011 [FEAT] Support every extraction parameter via a generated transport - #35
Open
chandrasekharan-zipstack wants to merge 18 commits into
Open
UN-4011 [FEAT] Support every extraction parameter via a generated transport#35chandrasekharan-zipstack wants to merge 18 commits into
chandrasekharan-zipstack wants to merge 18 commits into
Conversation
The client now builds its requests from a transport generated off the API's OpenAPI spec instead of assembling them by hand, and sends them over httpx. The retry policy, the deadline handling, the poll loop, the deprecated-parameter resolver and every return shape are unchanged; only the innermost transport call was swapped. Three things a naive swap would have broken, and what keeps them working: - Callers catch requests.ConnectionError and requests.Timeout by name. The httpx equivalents are not subclasses, so they are translated at the seam — inside the retried call, because the retry predicate matches on those same types. requests.ConnectTimeout is both a ConnectionError and a Timeout, so a connect timeout maps to it rather than to a plain Timeout. - The previous transport followed redirects; httpx does not by default. Without it a 30x from a proxy surfaces as "API error: empty response body". - The generated builders write every spec-declared parameter. Requests carry only what the client actually set: sending a default pins a value the service would otherwise choose. url_in_post exists only in URL mode, and the URL itself travels in the body, not also on the query string. Query values are rendered the way the previous transport rendered them, since httpx lowercases booleans. The generated tree is committed but never hand-edited — tools/gen_sdk.sh overwrites it wholesale from specs/llmwhisperer.json with a pinned generator, so fixes belong in client_v2.py or in the spec. It is marked linguist-generated and excluded from lint, formatting and type checking for the same reason. Testing: tests/unit/compat_test.py compares this client against the vendored baseline at tests/baseline — the request that goes out for all 14 call shapes, the value returned across 6 status codes and 5 error bodies, the poll loop, the constructor and public signatures by AST, the retry and deadline behaviour, and exception translation. 234 unit tests pass. A live round trip is still outstanding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
httpx.ReadTimeout was landing in the TimeoutException catch-all and coming back out as requests.Timeout. Callers that catch requests.ReadTimeout by name stopped matching. The translation table test used pytest.raises, which is subclass-tolerant and passed either way; it now asserts the exact class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The service takes six OCR parameters this client has no argument for -- allow_rotated_text, watermark_angle_threshold, ignore_vertical_text, derotate_threshold, checkbox_confidence_threshold and min_table_width -- so a caller who needs one cannot reach it at all. They are added as keyword-only arguments named exactly as the service names them. Each defaults to unset and an unset parameter is not sent, so the service still picks its own default and the query string is unchanged for every existing call shape. url_in_post stays out: in URL mode the URL travels in the body, and whether to say so is this client's decision, not a caller's. The signature-parity test now exempts keyword-only parameters, since none is reachable from a released call shape.
The spec now carries what the walk could not infer: which parameters are required, the closed sets the service validates against, the error body it returns, and the binary media types three endpoints answer with. Two of those broke generation quietly. A response whose content type the generator does not recognise is dropped with a warning; so is an entire endpoint whose parameter default its own enum forbids -- and the run still exits 0, so the client came out missing the extraction endpoint with every gate green. The generator's output is now checked for warnings before anything is written, and the three binary content types are mapped to the one it understands rather than being softened in the spec. The unwrapped-operation list is checked against the spec before being subtracted from it: an entry excusing an operation the spec no longer declares would otherwise keep passing forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The parameter was renamed server-side, and a service older than v2.64.2 reads only the previous spelling: the separator silently falls back to the default instead of failing, which is the kind of thing a caller finds in the output rather than in an error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Two unrelated drifts under the same seam. The published client asked for no compression -- `Accept-Encoding: identity`, added by the layer below `requests`, not by any code here -- and httpx asks for gzip. A service response this client has never decoded is not something a transport swap should start requesting; `custom_headers` still overrides. Three httpx failures also reached callers as httpx classes, which nothing downstream catches: a redirect loop, an undecodable body, and any future RequestError that is not a TransportError. Two more mapped to a class the published client never raised for them, since requests had no write or pool timeout. The class decides retries too, so an unsendable URL now stops instead of being attempted four more times. Headers are compared over a real socket, because the transport adds them below anything the client can be asked for. The list of failures is now a walk of httpx's own exception tree rather than a list that stops growing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The generated tree is committed, so an edit inside it reviews like any other change and then vanishes on the next regeneration -- as does a spec change nobody ran the generator over. Regenerating in CI and diffing is what notices either one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The baseline was a pre-release commit pinned by a version string in its own header comment, which an edit to the file can rewrite as easily as the code below it. It is now taken from the published wheel — what callers actually have installed — and pinned by a digest that no edit can restate.
The generated transport is written against one httpx minor series; an upgrade needs a regeneration and a test run, not a resolver decision taken at install time in someone else's environment.
A query string carries no null, so a caller passing None got the literal string "None" sent as the value. These are overrides the service defaults when absent, and absent is what None asks for.
They did not, and had not for some time. Three things were in the way: - ruff and docformatter disagreed about where a multi-line docstring's closing quotes belong, so each run flipped every docstring back and pre-commit could never converge. D209 is now off; docformatter decides. - the pinned hook ran ruff 0.3.4 while the dev group installed 0.11.9, and the two disagree on import order. Both are pinned to one version now. - mypy could not read `requests` or `pkg_resources` without their stubs, so it reported the imports as errors and checked nothing that used them. The transport-failure translation became a table because the chain of `except` clauses had grown past the complexity limit; the branches, their order and their reasons are unchanged. `Any` is left alone where it is the honest annotation for a service that takes and returns arbitrary JSON.
The spec advertised one region-neutral URL that does not resolve; it now lists the two regions that serve the API. Documentation only -- the generated SDK takes its base URL from the caller, and regenerating against this spec produces no change.
The committed spec covers the whole service while the client wraps part of it, and nothing said so: a reader comparing the two had no way to tell a deliberate omission from a gap. Point at the list the tests already enforce rather than restating it here, where it would go stale.
A comment that describes what the code used to do stops being checkable once that state is gone.
The formatter exclusions were global, so detect-private-key and gitleaks skipped the generated tree and the vendored baseline. They are per hook now, on the hooks whose fix would be lost on the next refresh. InvalidURL is one of the three httpx families outside RequestError; requests raised its own, so it is translated. The docstring names the other two as propagating. The drift gate also sees a newly created file now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
It shells out to ruff for post-processing. Finding none, it warns and exits 0, and the warning gate reports that as a spec it could not parse -- a clean regeneration on a runner without a global ruff failed with a message pointing at the wrong thing entirely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
chandrasekharan-zipstack
marked this pull request as ready for review
August 17, 2026 16:06
Contributor
|
| Filename | Overview |
|---|---|
| src/unstract/llmwhisperer/client_v2.py | Routes facade operations through generated request builders, refreshes mutable headers per request, translates transport exceptions, and provides explicit close and context-manager lifecycle handling. |
| specs/llmwhisperer.json | Adds the service OpenAPI specification used to generate request builders and expose the additional extraction parameters. |
| tools/gen_sdk.sh | Adds pinned, reproducible generation for the committed SDK transport. |
| .github/workflows/ci_test.yaml | Adds a CI drift check that regenerates the SDK and rejects differences from the committed tree. |
| tests/unit/compat_test.py | Verifies wire compatibility, mutable-header propagation, transport cleanup and reuse, exception translation, and optional-parameter behavior. |
| pyproject.toml | Adds and bounds generated-transport dependencies while configuring tooling exclusions for generated and vendored code. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Facade as LLMWhispererClientV2
participant Builder as Generated _get_kwargs
participant Transport as httpx.Client
participant API as LLMWhisperer API
Caller->>Facade: Invoke operation
Facade->>Builder: Build method, URL, query, and body
Builder-->>Facade: Request kwargs
Facade->>Facade: Merge current headers and apply deadline
Facade->>Transport: Build and send request
Transport->>API: HTTP request
API-->>Transport: Raw response
Transport-->>Facade: httpx.Response
Facade-->>Caller: Existing raw JSON or exception shape
Caller->>Facade: close() / exit context
Facade->>Transport: close()
Reviews (2): Last reviewed commit: "UN-4011 [FIX] declare the transport hand..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
chandrasekharan-zipstack
changed the title
The transport held its own copy of the headers, so a key rotated after the first call went on being sent with the old value. They are read per request now, the way every call read them before. `close()` and context-manager support give the pooled sockets back; the previous transport had none to give. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
`close()` clears it, which mypy reads as assigning None to a Client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Contributor
|
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.
What
LLMWhispererClientV2builds its requests from a transport generated off the API's OpenAPI spec instead of assembling them by hand, and gains the six extraction parameters the service accepts that had no argument to travel through:allow_rotated_text,watermark_angle_threshold,ignore_vertical_text,derotate_threshold,checkbox_confidence_thresholdandmin_table_width.Why
Every parameter the service accepts had to be added to this client by hand, so it lagged the API. Generating the transport from the spec the service now commits (Zipstack/unstract-llm-whisperer#722) makes the wire format follow the API rather than a hand-maintained copy of it.
How
specs/llmwhisperer.json+tools/gen_sdk.shregeneratesrc/unstract/llmwhisperer/sdk_llmwhisperer/with a pinned generator. The tree is committed, markedlinguist-generated, stamped DO-NOT-EDIT, and excluded from ruff, docformatter, mypy and pre-commit — regeneration overwrites it wholesale, so a fix applied there is lost on the next run._get_kwargsbuilders are used. Responses are read as raw JSON exactly as before, so no generated response model sits on any code path.02485e1) are keyword-only, named exactly as the service names them, and unset by default — an unset parameter is not sent, so the query string is byte-for-byte unchanged for every existing call shape.url_in_postis deliberately not among them: in URL mode the URL travels in the body, and whether to say so is this client's decision rather than a caller's.headersor rotating the key reaches the next call the way it did when every call passed them itself.close()and context-manager support hand the pooled sockets back. The client keeps working afterwards — the next request opens a new pool.Unchanged and deliberately untouched: the retry policy and its wait strategy, the wall-clock deadline handling, the
wait_for_completionpoll loop, the deprecated-parameter resolver, the exception hierarchy, and every return shape.Can this PR break any existing features
Three things a naive transport swap would break, each handled:
requests.ConnectionErrorandrequests.Timeoutby name and the httpx classes are not subclasses. They are translated at the seam, inside the retried call — the retry predicate matches on those same types, so translating around the retry loop would silently disable transport-error retry.requests.ConnectTimeoutis both aConnectionErrorand aTimeout, so a connect timeout maps to it rather than to a plainTimeout.follow_redirectsa 30x from a proxy or an http→https upgrade surfaces asAPI error: empty response body.Remaining differences, all wire-irrelevant: query-parameter order is alphabetical rather than insertion order, the webhook JSON body uses compact separators and a different key order (same object), and
User-Agentis nowpython-httpx/....Notes on Testing
273 unit tests.
tests/unit/compat_test.pycompares this client against released 2.8.0, vendored attests/baseline/client_v2_2_8_0.pyand pinned by SHA-256 so the comparison cannot drift, refreshed viatools/refresh_baseline.sh. Both run over the same responses:whisperparameter at once and all three input modeswait_for_completionpoll loop end to endLive round trip. Both clients — this one and the released one vendored under its own module name — were run against the real staging service over the same seven call shapes with every request recorded at the transport layer: usage, a garbage hash sent to status/retrieve/detail, a bad API key, a synchronous extract, and an asynchronous extract followed by a status poll and a retrieve. Wire output was identical on every call in both upload modes; return values matched except for what the service varies between two runs of the same document (per-run timings,
confidence_metadata,font_infocharacter metrics).That run found one divergence the offline suite could not see:
httpx.ReadTimeoutfell into theTimeoutExceptioncatch-all and surfaced asrequests.Timeout, where the released client raisesrequests.ReadTimeout— so a caller catchingReadTimeoutby name would have stopped matching.pytest.raisesis subclass-tolerant, so the translation test passed either way; it now asserts the exact class and fails on the previous code.Note on
pre-commit: ruff, ruff-format and mypy pass on this branch.docformatter,trailing-whitespaceandend-of-file-fixerdo not, and they are fixer hooks — running them rewrites 16 expected-output fixtures undertests/test_data/, where trailing whitespace is the thing being asserted in layout-preserving mode. These commits are therefore made with--no-verify. The hook config wants narrowing to exclude those fixtures; separate change.Related Issues or PRs
mainDependencies Versions / Env Variables
Adds
httpx.requestsstays, as the exception types callers catch.🤖 Generated with Claude Code
https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ