fix(llm): slice oversized PDFs through the converter (#2906) - #2907
fix(llm): slice oversized PDFs through the converter (#2906)#2907abhay-codes07 wants to merge 1 commit into
Conversation
) _read_files caps every unit at _FILE_CHAR_CAP (20,000 characters). expand_oversized_files exists so an oversized document is sliced and still reaches the model whole -- but it measured candidates with path.read_text() and read_slice_text sliced the same way, while llm._file_to_text routes a PDF through extract_pdf_text. Slicing a PDF would therefore have indexed the container's bytes instead of its text, so PDFs were excluded from slicing entirely and simply lost everything past 20,000 characters. That is the half Graphify-Labs#2900/Graphify-Labs#2902 called out and deliberately left alone. Papers are the longest documents anyone points graphify at, and a compressed PDF gives no hint of its text length: the fixture here is 3,094 bytes on disk and 55,690 characters of text. before: units=1 chars reaching the LLM=20000 LOST=35690 (64% of the paper) after: units=3 chars reaching the LLM=55690 LOST=0 unit_source_text is the fix: one reader returning the string the prompt will carry, used by BOTH the boundary pass and the slice reader, so the offsets a FileSlice holds always index the same string the model is shown. Plain text is still read straight off disk; only converted types go through a converter. is_splittable_text now answers "can this be addressed by character offset", which is the question slicing actually asks -- images and code stay out, PDFs come in. The reader memoises on (path, size, mtime_ns): slicing asks for the same file once to measure it and then once per slice, and extraction is the expensive part, so a corpus of papers is parsed once per run rather than once per slice. Keying on size and mtime means a paper replaced mid-run is re-read rather than sliced against stale text. Bounded, because the entries are whole documents. extract_pdf_text is imported lazily from detect: llm imports file_slice, so the reverse direction at import time would be a cycle. Composes with the estimator: a PDF over the cap is now a set of FileSlices, and _estimate_file_tokens already routes a slice through read_slice_text, so its token estimate becomes correct here too. A PDF UNDER the cap stays a Path and is still estimated from its bytes -- that is Graphify-Labs#2903/Graphify-Labs#2904, which is independent and does not touch this code.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Slices oversized PDFs against their extracted text instead of losing everything past the 20,000-character cap. Adds unit_source_text — a single reader (memoised on path/size/mtime) shared by expand_oversized_files and read_slice_text so slice offsets index the same string the prompt carries — and extends is_splittable_text to cover .pdf. Includes tests/test_pdf_slicing.py covering tiling, the cap, corrupt/rewritten PDFs, and unchanged plain-text/image/code behavior.
Worth a look
- bisect_slice reads PDF bytes instead of extracted text, misaligning slice offsets —
graphify/file_slice.py:208· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- bisect_slice re-reads PDF bytes instead of extracted text, contradicting new slicing contract —
graphify/file_slice.py:217· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- Converted PDF extraction exceptions escape oversized expansion —
graphify/file_slice.py:178· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- PDF converter failures are not handled during expansion —
graphify/file_slice.py:184· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
- PDF slices are bisected using raw file bytes instead of converted text —
graphify/file_slice.py:208· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 295 functions depend on the 44 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_corpus_parallel()— 26 callers, 10 callees - new:
dispatch_command()— 2 callers, 117 callees - new:
_extract_with_adaptive_retry()— 18 callers, 10 callees - new:
expand_oversized_files()— 20 callers, 4 callees - new:
_read_files()— 8 callers, 6 callees - new:
_pack_chunks_by_tokens()— 10 callers, 3 callees - new:
_bind_node_evidence()— 4 callers, 3 callees - …and 1 more — each is listed as a finding
Verification — 295 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 149 function(s) in the blast radius were not formally verified this run
Formal verification
No difference found (not proven): No behavior difference found in expand\_oversized\_files (not a proof).
The verifier ran both versions of expand\_oversized\_files on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify is\_splittable\_text.
The verifier did not have enough to check is\_splittable\_text, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify read\_slice\_text.
The verifier did not have enough to check read\_slice\_text, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `fs` is annotated `FileSlice` — outside the synthesizable primitive/collection set
· 9 more finding(s) on lines outside this diff (see the check run).
Fixes #2906.
This is the half #2902 called out and deliberately left undone.
The bug
expand_oversized_filesslices oversized documents so the whole file still reaches the model. But it measured candidates withpath.read_text()andread_slice_textsliced the same way, whilellm._file_to_textroutes a PDF throughextract_pdf_text. Slicing a PDF would therefore have indexed the container's bytes instead of its text — so PDFs were excluded from slicing entirely, and simply lost everything past_FILE_CHAR_CAP.One page, real text layer, FlateDecode as every real PDF is:
Nothing reports it — no warning, and no
_partial_filesmarker, since that machinery tracks truncated responses rather than truncated inputs. A paper whose second half was cut is indistinguishable in the graph from one whose second half had nothing extractable in it.A compressed PDF also hides the gap from every obvious check: 3,094 bytes on disk against 55,690 characters of text. Neither the file listing nor the corpus word count shows anything unusual.
The change
unit_source_text— one reader returning the string the prompt will carry, used by both the boundary pass and the slice reader. That is the whole fix: the offsets aFileSliceholds and the string the model is shown are now the same string by construction, rather than by two call sites happening to agree.Plain text is still read straight off disk; only converted types go through a converter, so nothing changes for the formats that already worked.
is_splittable_textnow answers the question slicing actually asks — can this be addressed by character offset? — instead of is this one of five suffixes. Images and code stay out; PDFs come in.extract_pdf_textis imported lazily fromdetect, becausellmimportsfile_sliceand the reverse direction at import time would be a cycle.The memo
Slicing asks for the same file once to measure it and then once per slice, and extraction is the expensive part — so the reader memoises on
(path, size, mtime_ns). A corpus of papers is parsed once per run rather than once per slice. Keyed on size and mtime so a paper replaced mid-run is re-read rather than sliced against stale text (there is a test), and bounded because the entries are whole documents.How it sits with the other two
.qmd,.html,.yaml,.skill) and explicitly deferred this. No code overlap — that PR only edits the suffix set, this one adds the reader._estimate_file_tokensreading a PDF's compressed bytes. Independent, and they compose: a PDF over the cap is now a set ofFileSlices, and_estimate_file_tokensalready routes a slice throughread_slice_text, so its estimate becomes correct here too. A PDF under the cap stays aPathand is still estimated from bytes — that is what fix(llm): estimate PDF tokens from the extracted text, not the container (#2903) #2904 fixes, and it does not touch this code.If all three land, the "raw file vs converted text" confusion is gone from the input path. Happy to unify the two memos (
_pdf_text_for_estimateinllmand this one) into a single shared reader afterwards — I kept them separate so each PR stands alone onv8.Tests
tests/test_pdf_slicing.py(13 tests). The fixture builds a real one-page PDF with a genuine text layer and first asserts the precondition — small on disk, large in text — since that shape is why file size never revealed the problem.Then: the PDF is splittable, an oversized one is sliced, the slices tile the extracted text exactly (
"".join(read_slice_text(u)) == unit_source_text(p)— gap-free, no overlap), no slice exceeds the cap, a slice containsSection 0and not%PDF/endstream(the bug in one assertion), and the tail actually reaches the prompt.Plus the guards: a small PDF still passes through whole, plain-text slicing is unchanged, images and code are still not sliced, a corrupt PDF does not break the pass, a rewritten PDF is re-read, and repeated reads agree.
Reverting
file_slice.pyand keeping the tests fails 6 of 13. Theunit_source_textimport is deliberately guarded with a fallback so the file still collects on the pre-fix tree — otherwise the whole module would error and the count would tell you nothing about which behaviours are pinned.Validation
Windows 11, Python 3.12, branched off
b14b52e(0.9.47).15 failed, 4738 passed->15 failed, 4751 passed. Identical failure set — no regressions; the +13 are the new tests.