Skip to content

feat: NDJSON elements-file mode for partition (0.46.0) - #347

Open
badGarnet wants to merge 11 commits into
mainfrom
feat/ndjson-elements-file
Open

feat: NDJSON elements-file mode for partition (0.46.0)#347
badGarnet wants to merge 11 commits into
mainfrom
feat/ndjson-elements-file

Conversation

@badGarnet

@badGarnet badGarnet commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

What

Adds an opt-in NDJSON response mode to partition() that returns elements as a path to a file on disk instead of a parsed list, and ships it as 0.46.0.

from unstructured_client.general import PartitionAcceptEnum

res = client.general.partition(
    request=req,
    accept_header_override=PartitionAcceptEnum.APPLICATION_X_NDJSON,
)

try:
    with open(res.elements_file, encoding="utf-8") as f:
        for line in f:
            element = json.loads(line)
            ...
finally:
    os.unlink(res.elements_file)

PartitionResponse.elements_file is set instead of PartitionResponse.elements. The caller owns the file and must delete it. Requesting application/json remains the default and is entirely unchanged.

Why

On the split-PDF path the SDK rebuilt the whole document in memory in order to return it: a list per chunk, a flattened list, a json.dumps blob in create_response, and then the SDK's re-parse of that blob — four copies live at once, with the serialization step dominating peak usage. For documents with large metadata.image_base64 payloads this is the difference between a job completing and being OOM-killed.

In the new mode the per-chunk temp files are concatenated on disk and never parsed, so peak memory is roughly one chunk rather than the whole document.

How

  • combine_chunk_files_to_ndjson concatenates chunk files on disk. Each chunk is sniffed for its first non-whitespace character, so a server returning application/json still works; chunks that are already NDJSON are copied through without parsing.
  • ndjson_mode depends only on the Accept header, never on split_pdf_cache_tmp_data. Those are set by different parties, so gating on both let them disagree — the server would return NDJSON while the hook took the JSON path and res.json() raised on a body this client had itself requested.
  • Both caching modes are handled. A cached chunk contributes its existing temp-file path; an uncached one spills its body verbatim and then releases it, since every response is retained in api_successful_responses and leaving _content set would keep the document resident regardless.
  • The combined file is deliberately written outside the operation's TemporaryDirectory, which _clear_operation removes as soon as after_success returns.

Temp-file ownership

Everything this path creates is accounted for:

  • Spilled chunk bodies are written inside the operation's temp directory and unlinked once combined.
  • The combined file is deleted when a chunk failure means it is never handed back to the caller.
  • Recombination writes to a staging file that is atomically renamed into place only on success, so a malformed chunk cannot orphan a partial file.
  • No combined file is created at all when every chunk failed.

Security

The elements-file marker is an httpx response extension, not a response header. Extensions are populated by the transport, so a remote server cannot set the key. A header would be wire-controlled, and since callers are documented to open elements_file and then delete it, that would hand a hostile server an arbitrary local file to destroy. A real server body is always copied to a file this client creates.

Regeneration

elements_file is client-side only and can never come from the OpenAPI spec, so a regeneration would silently drop it. Both general.py and models/operations/partition.py are now in .genignore, and test_regeneration_guards.py fails if either entry is lost.

Known limitation

elements_file is set for every input, so callers need one code path. The memory saving, however, applies only to split PDFs.

An input is sent whole when it is not a PDF, when split_pdf_page=False, or when it has two pages or fewer — _before_request_unlocked short-circuits on split_size >= page_count and get_optimal_split_size floors at MIN_PAGES_PER_SPLIT = 2. For those, the body is read fully into memory before being written to disk, so peak is roughly 2x the body rather than bounded.

Bounding it means stream=True for NDJSON requests, which makes raw_response.content raise on the returned closed response — a user-visible change worth its own review. Tracked separately.

Note also that the deployed API does not currently emit application/x-ndjson, so the unsplit path reaches the JSON-to-NDJSON conversion rather than the streamed-body branch. That is not merely a spec omission: the service does not negotiate the response format on Accept at all. It selects the format from the output_format form field, and consults Accept only to choose multipart/mixed and to reject conflicting media types on multi-file uploads. NDJSON was therefore never going to arrive via Accept.

The service's 406 NOT_ACCEPTABLE on an unrecognized Accept is gated on multi-file uploads. This SDK sends a single file per request — PartitionParameters.files is one Files, and the split-PDF hook sends one chunk per request — so that branch is unreachable from here and the unsplit path cannot raise SDKError because of it. Server-side NDJSON support is tracked separately.

Testing

  • New _test_unstructured_client/unit/test_ndjson_elements_file.py — recombination across JSON-array / NDJSON / mixed chunk formats, order preservation, byte-exact payload round-trip, non-ASCII, temp-file lifecycle on success and failure, and regression guards for the header-spoofing and partial-output defects.
  • 235 unit tests and 64 contract tests pass; pylint 10.00/10; mypy clean.

🤖 Generated with Claude Code

Review in cubic

badGarnet and others added 3 commits August 1, 2026 13:04
On the split-PDF path the SDK rebuilt the whole document in memory to return it:
a list per chunk, a flattened list, a json.dumps blob in create_response, and the
SDK's re-parse of that blob -- four copies live at once, with the serialization
step dominating peak usage on large documents.

Passing accept_header_override=PartitionAcceptEnum.APPLICATION_X_NDJSON now
returns PartitionResponse.elements_file -- a path to an NDJSON file, one element
per line -- instead of PartitionResponse.elements. The per-chunk temp files are
concatenated on disk and never parsed, so peak memory is roughly one chunk rather
than the whole document.

Chunk files are sniffed for their first non-whitespace character, so a server
returning application/json still works; NDJSON chunks are copied through
untouched. Requesting application/json remains the default and is unchanged.

The caller owns the returned file and must delete it. It is deliberately written
outside the operation's TemporaryDirectory, which _clear_operation removes as soon
as after_success returns.

ndjson_mode depends only on the Accept header, never on split_pdf_cache_tmp_data.
Those are set by different parties, so gating on both let them disagree: the
server would return NDJSON while the hook took the JSON path and res.json() raised
on a body this client had itself requested. Both caching modes are handled -- a
cached chunk contributes its existing temp-file path, an uncached one spills its
body verbatim and then releases it, since every response is retained in
api_successful_responses and leaving _content set would keep the document resident
regardless.

general.py and models/operations/partition.py are both .genignore'd: elements_file
is client-side only and can never come from the OpenAPI spec, so a regeneration
would silently drop it. test_regeneration_guards.py fails if either entry is lost.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Two defects found in review of the elements-file mode.

The elements-file marker was an `x-unstructured-elements-file` response header, and
any response carrying it was trusted as an SDK-created path. Headers come off the
wire, so a server could name an arbitrary local file -- and callers are documented
to open `elements_file` and then delete it, making this an arbitrary-file delete
rather than just a disclosure. The marker is now an httpx response extension, which
is populated by the transport and cannot be set remotely; a real server body is
always copied to a file this client creates.

Recombination also wrote straight to its final UUID path while recording that path
only on success, so a malformed chunk left a partial file behind under a name
nothing owned -- the combined file is deliberately outside the operation's
TemporaryDirectory, so nothing else cleaned it up. It now writes to a staging file
renamed into place atomically, and unlinks it on any exception.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Ships the NDJSON elements-file mode.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 12 files

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread src/unstructured_client/general.py Outdated
Comment thread docs/models/operations/partitionresponse.md
Comment thread src/unstructured_client/_hooks/custom/request_utils.py
Comment thread src/unstructured_client/_hooks/custom/split_pdf_hook.py Outdated
Comment thread README.md Outdated
Comment thread README.md
Comment thread _test_unstructured_client/unit/test_ndjson_elements_file.py Outdated
Addresses review findings on the elements-file path.

Orphaned files on failure. Both `_ndjson_elements_file` helpers create their
destination with delete=False, so a body that raises or is cancelled partway
through left the partial copy behind; they now unlink it and re-raise.
`write_chunk_body_to_temp` had the same shape -- the caller only registers the
path for cleanup once the function returns, so a failed write orphaned the file,
and a full disk is exactly the failure that repeats.

Cancellation race. Recombination runs in a worker thread that cancellation cannot
interrupt, so `_clear_operation` could tear the operation down while it was still
running; publishing the path afterwards resurrected a cleared dict entry and
orphaned the file. Publishing is now gated on the operation still being live,
under a lock that `_clear_operation` also takes when dropping
`pending_operation_ids`. Ownership is explicit either way: the success path claims
the path out of `ndjson_output_path`, so anything still recorded at teardown was
never delivered and is safe to delete.

Docs regeneration. docs/models/operations/partitionresponse.md is generated and
tracked in gen.lock, and the generation workflow runs on a daily cron, so the
elements_file row would have been dropped within a day of merging. Added to
.genignore alongside the model, and the regeneration guard now asserts the row.

README. Noted that the memory saving applies to the split-PDF path -- unsplit
inputs still buffer the body -- so the caveat is visible where the feature is
advertised rather than only in the PR. The example's cleanup used a bare unlink in
a finally, which would mask a failure to open the file with FileNotFoundError; it
now uses Path.unlink(missing_ok=True).

The spilled-body regression guard asserted against `_content` it had assigned
itself, so it could not fail if the hook stopped releasing the body. It now drives
`_elements_from_task_responses`, and was confirmed to fail with the release
removed.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 7 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread src/unstructured_client/_hooks/custom/request_utils.py
The unlink-on-failure paths added in the previous commit had no coverage -- the
existing tests only walked the success roundtrip, so the cleanup could have been
removed without anything going red.

Three tests, each confirmed to fail with its corresponding unlink removed:

- write_chunk_body_to_temp: os.fdopen is patched so the write raises ENOSPC. The
  wrapper still closes the real handle, so the fd is not leaked by the test itself.
- _ndjson_elements_file and its async counterpart: a response whose byte iterator
  raises partway through. tempfile.tempdir is redirected at the test's tmp_path so
  the assertion can see whether anything was left behind.

Each asserts both halves of the contract: no file survives, and the original
exception still propagates rather than being swallowed by the cleanup.

Coverage for the general.py pair was not requested in review, but those helpers
grew the same delete=False cleanup in the same commit and had the same gap.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 1 file (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread _test_unstructured_client/unit/test_ndjson_elements_file.py
The orphan checks asserted that a directory held no matching files, which only
proves cleanup if the file landed in that directory to begin with. That held solely
because the helpers route through the global tempfile.tempdir the tests patch --
an assumption the tests never checked. A change that passed an explicit dir would
have made them pass while a partial file leaked into the real temp dir.

A shared `_record_created_paths` spy now wraps the temp-file factory, delegating to
the real one and recording each path it hands out. The tests assert on that path:
one file was created, it was where the test expected, and it is gone.

Verified by breaking it two ways. With the unlink removed the tests fail, as
before. With the unlink removed AND the destination pinned to an explicit dir that
ignores the patched tempdir -- the vacuous case, which the old directory-glob
assertion passed -- they now fail too.

Applied to the spill test as well. Review only raised the two copy-failure tests,
but that one asserted on an empty directory for the same reason and could go
vacuous the same way if `dir_` stopped being honored.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 1 file (changes from recent commits).

Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic

@awalker4 awalker4 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.

Went through this against a local checkout — 241 unit tests pass, pylint 10.00/10, mypy clean on the touched modules, so the numbers in the description hold up.

The split-PDF path is the real substance here and it's well executed. A few things stand out as better than average:

  • ELEMENTS_FILE_EXTENSION_KEY as an httpx extension rather than a response header. Correct call, and the reasoning in the comment is right — a wire-controlled header would let a hostile server name any local path for a caller who is documented to open it and then delete it. Good that there's a test pinning it.
  • ndjson_mode keyed only off Accept, never off split_pdf_cache_tmp_data. The comment in _before_request_unlocked describes a genuine failure mode from two independently-set knobs disagreeing.
  • The cancellation design holds up. I walked both interleavings of the publish in _combine_chunks_to_ndjson against teardown in _clear_operation: staging file, os.replace, and pending_operation_ids checked under _ndjson_lock means the combined file is either claimed by the caller or deleted, never both and never neither. _claim_ndjson_output / _discard_ndjson_output is a good way to name that.
  • Releasing res._content after spilling, with a test that drives the hook rather than the helper. That's a real fix to the earlier vacuous-guard complaint, not a cosmetic one.

Three things I'd want addressed, left as inline comments: the unsplit-path contract in general.py, the corresponding README claim, and the regeneration scaffolding (which I think can just come out — details inline).

Smaller notes, none blocking:

  • Empty chunks are silently dropped. _first_non_space_char returning "" leads to a continue, pinned as intended by test_empty_chunk_files_are_skipped. In JSON mode an empty chunk file makes json.load raise, so the caller learns the document is short; in NDJSON mode a 200 with an empty body yields a silently truncated document instead. A logger.warning per skipped chunk would at least let event=ndjson_combined element_count be reconciled against the chunk count.
  • The format sniff is [ versus "assume one JSON object per line." A multi-line pretty-printed JSON object would get split into invalid lines. Not reachable through the current endpoint, but it's an assumption rather than a check.
  • Mixed temp-file suffixes — cached chunks are .json, spilled ones .ndjson, which makes glob-based cleanup and the test helpers asymmetric.
  • _combine_chunks_to_ndjson's docstring is four paragraphs of design rationale, and the cancellation paragraph restates the comment on _ndjson_lock. Worth a trim.

Leaving this as a comment rather than a block — the asks are clear enough and I don't want to gate the release on process questions that are partly mine to answer.

Comment thread src/unstructured_client/general.py Outdated
content_type=http_res.headers.get("Content-Type") or "",
raw_response=http_res,
)
if utils.match_response(http_res, "200", "application/x-ndjson"):

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.

This is the one I'd most want changed before merge.

I checked the live spec rather than the vendored copy — https://api.unstructuredapp.io/general/openapi.json, currently v1.5.69. The 200 declares application/json and text/csv only, and the string ndjson appears nowhere in the document. So the deployed server does not emit application/x-ndjson today.

Which means on the unsplit path the real server response lands here, and the application/json branch above is checked first and wins:

content-type: application/json
  matches application/json     -> True
  matches application/x-ndjson -> False

elements gets populated and elements_file stays None, even though the caller explicitly asked for NDJSON.

Worth stressing how wide "unsplit" is, because it isn't just non-PDFs. split_size >= page_count short-circuits in _before_request_unlocked, and with MIN_PAGES_PER_SPLIT = 2 that catches every single-page PDF, plus anything with split_pdf_page=False. So this surfaces on small inputs and will read as intermittent to whoever hits it.

Knock-on: the "Known limitation" section in the description describes a path that never executes against the current API, and the server-body branch of _ndjson_elements_file / _ndjson_elements_file_async is effectively dead code until the API ships NDJSON.

What I'd prefer: when NDJSON was requested and the server returned JSON, spill that body to a file and set elements_file anyway. That keeps the documented contract true regardless of server support, and it means the mode behaves the same way for every input. The alternative is to document that elements_file can be None and guard the README example, but then callers need two code paths for one opt-in flag.

Also worth confirming with whoever owns the API: if the server 406s on an unrecognized Accept rather than ignoring it, the unsplit path raises SDKError instead, which is a different problem again.

Related: nothing currently drives client.general.partition(accept_header_override=APPLICATION_X_NDJSON) end to end. All 24 new tests are helper- or hook-level. One mock-transport test returning application/json would have caught this.

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.

Walking back the framing here — I called this the one I'd most want changed, and the premise underneath that is weaker than I made it sound.

I inferred "the server doesn't emit NDJSON" from the live /openapi.json. But that document is generated from the route's explicit responses={} annotations, so a runtime content-negotiation branch returning a StreamingResponse(media_type="application/x-ndjson") wouldn't necessarily show up in it. Absence there isn't absence in the server. And if you're building the client half, the reasonable assumption is that the server half is already handled — I should have asked instead of inferring from a generated artifact.

What does survive is narrower and doesn't depend on the server at all: general.py checks application/json before application/x-ndjson, so for any deployment that answers an NDJSON request with a JSON body, elements_file comes back None. That isn't hypothetical even with full support in prod — this SDK gets pointed at self-hosted and older unstructured-api images, which I'd guess is part of why combine_chunk_files_to_ndjson sniffs for [ on the chunk path in the first place. The unsplit path just doesn't have the equivalent of that sniff.

So the actual question, downgraded from "want changed" to "want confirmed": do you want the unsplit path to degrade silently to elements against an older server, or to spill the JSON body to a file so elements_file is always set? Either is defensible — I'd just rather it were a decision than an accident.

If there's a server PR landing NDJSON on /general/v0/general, point me at it and the only thing left in this thread is the README wording.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 358e21a, taking the option you preferred: elements_file is now set whether or not the server honors the header. The application/x-ndjson branch is checked first, and a JSON body is written out as NDJSON when NDJSON was requested, so callers need one code path.

I reproduced your spec finding independently before changing anything — v1.5.69, 200 declares application/json and text/csv only, ndjson absent from the whole document.

One correction to the scope, which is slightly wider than "every single-page PDF": get_optimal_split_size returns max(ceil(pages / concurrency), MIN_PAGES_PER_SPLIT) and the short-circuit is split_size >= page_count, so with the floor at 2 a two-page PDF is also sent whole. The README caveat and the PR description now both say "two pages or fewer".

I was deliberately explicit in the docstring and changelog that this conversion does not bound memory — the body is already read by the time it runs, and parsing adds the element list on top. It makes the contract uniform, nothing more. Overstating that is what produced the original problem.

On the missing coverage: three tests now drive partition() over a mock transport — server returns JSON, server returns NDJSON, and no override at all. I verified the first fails against the old dispatch. Aside for anyone writing similar tests: hold the client for the duration of the call, because sdk.py registers a weakref.finalize that closes the transport, so chaining off a temporary fails with a confusing 'NoneType' has no attribute 'build_request'.

On the 406 — I went and read the service, and it does not need confirming: this SDK cannot trigger it. The 406 NOT_ACCEPTABLE is gated on len(files) > 1. For a single file the check is skipped and the Accept value is never validated. This SDK sends one file per request — PartitionParameters.files is a single Files, and the split hook sends one chunk per request — so the unsplit path cannot raise SDKError because of it. Your caveat is real for direct multi-file API callers, just not reachable from here.

Reading that turned up something more consequential, though: the service does not negotiate response format on Accept at all. It selects the format from the output_format form field, and consults Accept only to pick multipart/mixed and for that multi-file guard. So NDJSON was never going to arrive via Accept regardless of what the spec declared — and the existing accept_header_override=TEXT_CSV does not make the server return CSV either; output_format does. I have corrected the docstring and the PR description, which both understated this as a spec omission, and filed the server-side work (add application/x-ndjson to output_format, emit it as a stream, update the spec, extend the multi-file allowlist) separately under the memory epic. Nothing in this PR depends on it.

Two follow-ups from a later automated pass, both fixed in ba3e7f4 and both touching this change: the conversion was iterating http_res.json() directly, so a {"detail": ...} body wrote its keys out as elements — it now goes through unmarshal_json_response. And detection keyed off accept_header_override alone while the split hook keys off the request header, so http_headers={"Accept": "application/x-ndjson"} produced exactly the split-vs-unsplit disagreement you were describing, on the same input. Both now read the same source.

Comment thread README.md Outdated
**You own the returned file and are responsible for deleting it.**

> [!NOTE]
> The memory saving applies to the split-PDF path, i.e. a PDF with `split_pdf_page=True` (the default). For unsplit inputs — a non-PDF file, or `split_pdf_page=False` — the response body is still read fully into memory before being written to disk, so peak memory can reach roughly twice the body size. You still get `elements_file` either way.

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.

This sentence isn't true against the current API — see the comment on general.py. For unsplit inputs the server returns application/json, the JSON branch matches first, and elements_file comes back None.

That makes the example below worse than a plain failure: open(res.elements_file) raises TypeError: expected str, bytes or os.PathLike object, not NoneType, and then the finally raises the same TypeError out of Path(None) and masks the original. So the first thing a user sees is a confusing double-TypeError from copy-pasted README code.

If the contract gets made uniform in general.py then this line is fine as written. If not, it needs to say elements_file may be None and the example needs an if res.elements_file: guard.

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.

Softening this one too — see the reply on general.py. My claim that the server returns JSON to an NDJSON request came from the live /openapi.json, which only reflects explicit responses={} annotations, so it does not settle what the server actually does.

The narrow version still stands: if elements_file can ever come back None — an older or self-hosted deployment, say — then this example raises TypeError on the open() and then raises the same TypeError again out of Path(None) in the finally, masking the first. That is a rough first experience for copy-pasted code.

If elements_file is always set against a current server, the sentence is fine as written and this is moot.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The contract was made uniform in general.py, so the sentence is now true as written — elements_file is always set when the header is passed.

I also corrected the caveat itself, which was wrong in a way you did not flag: it said "non-PDF, or split_pdf_page=False" and omitted small PDFs entirely. Because split_size is floored at 2 and the hook short-circuits on split_size >= page_count, one- and two-page PDFs are sent whole too. It now reads "two pages or fewer".

The double-TypeError is gone with the contract fix, and the example uses Path(...).unlink(missing_ok=True) so a failure to open the file is not masked by the cleanup.

Worth noting the same defect then turned up in my own tests — a later pass caught bare os.unlink in the finally blocks of the new end-to-end tests, masking assertion failures exactly as the README example would have. Fixed in ba3e7f4.

assert "run: make install" in workflow


def test_partition_response_keeps_elements_file():

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.

I think this test and the two new .genignore entries can come out — the regeneration they're defending against has been hard-blocked for months.

  • speakeasy_sdk_generation.yml has 100 consecutive failed runs, oldest in the queryable window 2026-04-26, newest today.
  • The failure isn't transient or spec-related: ##[error]generation access blocked, i.e. Speakeasy account level.
  • The last generated PR that actually merged was chore: 🐝 Update SDK - Generate 0.42.10 #329 on 2026-02-04. .speakeasy/gen.lock hasn't moved since 2026-01-18.

So no regeneration can drop elements_file, because no regeneration completes. That's roughly 38 lines that can go: this test, the 11 new .genignore lines, and the Regeneration section of the description.

If any of it does stay, the comments need rewording. Both this test and the .genignore entries assert that "the daily generation workflow would drop the row on its next run," which is now just false and will mislead the next person to read it. The test also string-matches paths inside .genignore, which is a gate that costs maintenance and catches nothing.

To be clear this is a call about where the repo is heading rather than a defect in your work — the premise you were given was reasonable, it just doesn't hold anymore. Given generation is blocked at the account level, the burden is on restoring Speakeasy rather than on defending against it. Happy to own that decision if it's easier; I'll follow up separately on whether that cron should just be turned off.

One knock-on if you do drop it: the PartitionAcceptEnum.APPLICATION_X_NDJSON assertion in here is currently the only test reference to the new enum, so it'd be worth folding that into whatever end-to-end test covers the mode.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I verified all three of your data points before touching anything: 100 consecutive failed runs (oldest in the queryable window 2026-04-26, newest today), ##[error]generation access blocked, and .speakeasy/gen.lock unchanged since 2026-01-18. Your conclusion holds — nothing can drop the field because nothing completes.

I have left the structural decision to you rather than removing it unilaterally, since you offered to own it and it is a call about where the repo is heading.

What I did do is fix the wording, which was false regardless of that decision. The .genignore comment and this test's docstring no longer claim an imminent regeneration; they now state that generation is blocked at the account level and that the entries are insurance for when it is restored. I also posted a correction on the partitionresponse.md thread, where I had asserted the row would disappear "within a day of merging" — I had checked that the cron existed without checking that the workflow succeeds, which is on me.

One update relevant to your decision: the PartitionAcceptEnum.APPLICATION_X_NDJSON assertion here is no longer the only test reference to the enum. The new end-to-end tests in test_ndjson_elements_file.py use it in five places, so if you drop this test the enum keeps its coverage and nothing needs folding in first.

For what it is worth, my own read is to keep the two .genignore entries — they are three lines and correct whenever generation returns — and drop the string-matching of .genignore contents from this test, since that part asserts on a config file rather than on behavior. But I am happy either way and will action whatever you decide.

The deployed API does not offer application/x-ndjson -- its spec (v1.5.69) declares
only application/json and text/csv, and the string "ndjson" appears nowhere in the
document. So any request that bypasses the split-PDF hook came back as JSON, the
application/json branch matched first and won, and elements_file stayed None while
elements was populated. The opt-in was silently ignored.

That path is much wider than "non-PDF". _before_request_unlocked short-circuits when
split_size >= page_count, and get_optimal_split_size floors at MIN_PAGES_PER_SPLIT=2,
so one- and two-page PDFs are sent whole -- it would have read as intermittent to
anyone hitting it on small inputs.

The x-ndjson branch is now checked first, and a JSON body is written out as NDJSON
when NDJSON was requested. elements_file is therefore set for every input and
callers need one code path rather than two. This does not bound memory on the
unsplit path -- the body is already read and parsing adds the element list on top --
it makes the contract uniform. The memory win remains the split-PDF path, where the
hook concatenates chunk files on disk and this conversion is never reached.

Also fixes the README example, which raised TypeError from open(None) and then
raised the same TypeError again out of Path(None) in the finally, masking the first.

Adds the end-to-end coverage whose absence let this through: all 24 existing tests
were helper- or hook-level, so nothing exercised the media-type dispatch in
general.py. Three tests now drive partition() over a mock transport -- server
returns JSON, server returns NDJSON, and no override at all. The first fails against
the old dispatch. They hold the client for the call rather than chaining off a
temporary, since sdk.py registers a weakref.finalize that closes the transport.

Smaller review notes: log a warning per skipped empty chunk so element_count can be
reconciled against chunk_count; document that the non-"[" sniff assumes one JSON
value per line rather than checking; trim the _combine_chunks_to_ndjson docstring.

The regeneration comments no longer claim a daily workflow is about to drop the
field -- generation is blocked at the Speakeasy account level and gen.lock has not
moved since 2026-01. Whether that scaffolding is worth keeping is left open.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 8 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/unstructured_client/general.py Outdated
Comment thread src/unstructured_client/general.py Outdated
Comment thread src/unstructured_client/general.py
Comment thread _test_unstructured_client/unit/test_ndjson_elements_file.py Outdated
Comment thread src/unstructured_client/_hooks/custom/split_pdf_hook.py Outdated
Four review findings on the elements-file path, all reproduced first.

The JSON-to-NDJSON conversion iterated `http_res.json()` directly, bypassing the
response schema. A `null` body raised TypeError from iterating None, and a non-array
body was worse than that: iterating a dict yields its keys, so `{"detail": "oops"}`
was written out as a one-element document reading `"detail"`. It now goes through
`unmarshal_json_response`, so a malformed 200 fails exactly as it does on the
`elements` path, and `null` yields an empty file.

NDJSON detection keyed off `accept_header_override` alone, but `http_headers` can
replace Accept too -- and the split-PDF hook already keys off the request header.
So one caller passing `http_headers={"Accept": "application/x-ndjson"}` got
`elements_file` for a multi-page PDF and `elements` for a two-page one. Detection now
reads the header off the built request, which is the same source the hook uses.

The async path ran the parse-and-write inline, blocking the event loop for exactly
the large bodies this mode exists for. Offloaded with `asyncio.to_thread`.

Test `finally` blocks used a bare `os.unlink`, which masks an assertion failure above
it with FileNotFoundError -- the same defect flagged in the README two rounds ago,
reintroduced in the tests. Now `Path(...).unlink(missing_ok=True)`.

Also removes a duplicated "The" left in `_combine_chunks_to_ndjson`'s docstring by
the earlier trim.

Four tests added, each confirmed to fail against the pre-fix source. The async one
originally asserted `asyncio.to_thread` was called, which passed even with the
offload removed because other SDK internals use it during the same call; it now
records the thread the conversion ran on and compares it to the event-loop thread.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 3 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/unstructured_client/general.py Outdated
The API does not negotiate response format on Accept at all: it reads the
output_format form field, and only consults Accept to select multipart/mixed and to
reject conflicting media types on multi-file uploads. Saying the spec merely omits
application/x-ndjson understated it -- NDJSON was never going to arrive via Accept
regardless of what the spec declared.

Comment only; no behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@badGarnet

Copy link
Copy Markdown
Collaborator Author

Thanks — this was a genuinely useful review. All three inline asks are addressed and answered in their threads; the four smaller notes are done too:

  • Empty chunks silently dropped. Agreed, and added event=ndjson_empty_chunk at WARNING per skipped chunk, so event=ndjson_combined element_count can be reconciled against the chunk count.
  • Format sniff is an assumption, not a check. Documented as exactly that in combine_chunk_files_to_ndjson, including that a pretty-printed multi-line object would be split into invalid lines and that no endpoint returns one today. I did not add validation: parsing every line to check it would undo the zero-parse fast path that is the point of the NDJSON branch.
  • _combine_chunks_to_ndjson docstring. Cut from four paragraphs to two, with the cancellation paragraph removed in favour of pointing at _ndjson_lock. (That trim left a duplicated "The", caught on a later pass.)
  • Mixed temp-file suffixes. Left alone deliberately: the cached .json name comes from the pre-existing chunk-caching path rather than this feature, so renaming it would touch code outside this change for cosmetic gain. Happy to do it if you would rather it be consistent now.

Two things worth surfacing from working through the review, both in the threads but easy to miss:

  1. The 406 question is answered, not open. The guard is gated on multi-file uploads and this SDK sends one file per request, so it is unreachable here. No confirmation needed from anyone.
  2. The service does not negotiate response format on Accept at alloutput_format does it. That makes your "the server does not emit application/x-ndjson" true for a deeper reason than the spec omission, and it means the pre-existing accept_header_override=TEXT_CSV does not drive the server either. Docstring and PR description corrected; server-side work filed separately under the memory epic. Nothing here depends on it.

Also fixed since your review, from an automated pass over the same code: the JSON conversion was iterating the raw body so a non-array 200 wrote a dict's keys out as elements; NDJSON detection ignored Accept set via http_headers, which reproduced the split-vs-unsplit disagreement you described; and the async path ran the conversion on the event loop.

Current state: 248 unit + 64 contract tests pass, pylint 10.00/10, mypy clean. The only open item is your call on the regeneration scaffolding.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 1 file (changes from recent commits).

Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic

Offloading the JSON-to-NDJSON conversion to a thread last commit removed the event
loop block but introduced a leak: a thread cannot be cancelled, so on cancellation
the conversion still runs to completion and creates its file, while the awaiting
coroutine has already raised CancelledError and discarded the path. Nothing was left
that could delete it. Reproduced -- cancel mid-conversion and an
unst_elements_*.ndjson survives.

The conversion is now awaited through asyncio.shield, which keeps a handle on the
thread's result after the caller stops waiting, and a done callback unlinks the
finished file. A conversion that raised needs no callback, since it already removes
its own partial file.

This is the same shape as the split hook's cancellation race: work that outlives the
operation that requested it, publishing a path nobody will read. It did not exist on
the inline version, which had no await point to cancel at.

Regression test asserts against the path production actually created, so it cannot
pass by observing that no file was ever made. Confirmed to fail against the plain
awaited to_thread.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="_test_unstructured_client/unit/test_ndjson_elements_file.py">

<violation number="1" location="_test_unstructured_client/unit/test_ndjson_elements_file.py:772">
P3: This cancellation test can race on slower or busy CI runs: the conversion may finish before `task.cancel()` because it relies on a fixed `time.sleep(0.3)`. Synchronizing the worker with `threading.Event` (signal started, then wait for release) would make the cancellation scenario deterministic instead of wall-clock dependent.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread _test_unstructured_client/unit/test_ndjson_elements_file.py Outdated
The test raced. It relied on a fixed `time.sleep(0.3)` in the worker outlasting an
`await asyncio.sleep(0.05)` before `task.cancel()`. On a loaded runner the conversion
could finish first, leaving nothing to cancel, and the test would fail with
"DID NOT RAISE" -- a spurious failure rather than a real one.

Now sequenced with three `threading.Event` handshakes: the worker signals that it has
entered the conversion, the test cancels only after that, and only then releases the
worker, so completion is necessarily post-cancellation. A spy on
`_discard_elements_file` signals that cleanup ran, so the assertion waits on the
actual event rather than polling the filesystem on a timer. The `wait` timeouts are
deadlock guards, never a duration anything waits out.

Also drops the 0.02s x 200 polling loop, so the test now takes ~0.15s instead of
~0.5s, and reports "cleanup never ran" instead of a bare assertion when it fails.

Still confirmed to fail against the plain awaited to_thread. Ran 20x clean, and 10x
clean under eight busy cores.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 1 file (changes from recent commits).

Shadow auto-approve: would require human review. Adds a new NDJSON mode and elements_file response field. Requires human sign-off on the public API expansion, the manual override of SDK generation via .genignore, and the design decision to delegate temp-file deletion to the caller.

Re-trigger cubic

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.

3 participants