fix(llm): estimate PDF tokens from the extracted text, not the container (#2903) - #2904
Conversation
…ner (Graphify-Labs#2903) _read_files sends a PDF through _file_to_text -> extract_pdf_text, but _estimate_file_tokens read the file with read_text and tokenised THAT. A PDF's bytes are not its text: every real PDF Flate-compresses its content streams, so the estimate measured a compressed binary. One 400-line fixture, identical text, stored two ways: uncompressed bytes=28470 est=5559 actual=4598 actual/est=0.83x FlateDecode (real PDFs) bytes= 1728 est=1334 actual=4599 actual/est=3.45x _pack_chunks_by_tokens sizes chunks from that estimate, so a 3.45x undercount overfills the chunk. The request then blows the model's context window and _extract_with_adaptive_retry bisects, paying for the same content two, four or eight times before it fits -- on a corpus of papers, which is exactly the corpus PDFs imply. Nothing looks broken from outside; it just costs several times what it should and takes correspondingly longer. After: both encodings estimate 4582 against 4598/4599 actual -- 1.00x, with the remaining gap being the wrapper overhead constant. Scoped to PDFs. Every other path still measures its own bytes, because for them read_text IS what the prompt carries. The tokenizer-absent branch keeps its size-based heuristic for non-PDFs and now uses the extracted length for PDFs, so the fallback stops being wrong in the same direction. _pdf_text_for_estimate memoises on (path, size, mtime_ns): packing probes the same file repeatedly while deciding where a chunk ends, and extraction is the expensive part, so a corpus of papers is parsed once per run rather than once per probe. Keying on size and mtime means a file rewritten mid-run is re-read rather than served a stale estimate, and the cache is bounded so a large corpus cannot pin every paper's text in memory. This is the same class as Graphify-Labs#2900: the estimate/slicing paths read the raw file while the prompt path goes through _file_to_text. That PR noted PDFs still need the converter threaded through read_slice_text; this fixes the independent half in token estimation, which costs money even when slicing never applies.
![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 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Fixes PDF token estimation in _estimate_file_tokens, which read a PDF's compressed bytes via read_text and undercounted by ~3.5x, causing _pack_chunks_by_tokens to overfill chunks and trigger adaptive bisection (#2903). Now estimates PDFs from their extracted text via a new _pdf_text_for_estimate helper memoised on (path, size, mtime) with a 512-entry bound. Adds tests/test_pdf_token_estimate.py covering estimate/actual parity, compression-invariance, cache freshness on rewrite, and the char cap.
Worth a look
- PDF estimate cache stores uncapped extracted text —
graphify/llm.py:1906· 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 — 732 functions depend on the 184 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
extract_corpus_parallel()— 26 callers, 10 callees - new:
dispatch_command()— 2 callers, 117 callees - new:
_call_llm()— 11 callers, 18 callees - new:
_call_openai_compat()— 23 callers, 8 callees - …and 16 more — each is listed as a finding
Verification — 732 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: 463 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_estimate\_file\_tokens.
The verifier did not have enough to check \_estimate\_file\_tokens, 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 `unit` is annotated `'Path | FileSlice'` — outside the synthesizable primitive/collection set
· 1 grounded finding(s) anchored inline below; 23 more finding(s) on lines outside this diff (see the check run).
| return text | ||
|
|
||
|
|
||
| def _estimate_file_tokens(unit: "Path | FileSlice") -> int: |
There was a problem hiding this comment.
_estimate_file_tokens()
15 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Fixes #2903.
The bug
_read_filessends a PDF through_file_to_text→extract_pdf_text._estimate_file_tokensread the file withread_textand tokenised that — so it measured a compressed binary, because every real PDF Flate-compresses its content streams.One 400-line fixture, identical text, stored two ways:
estis_estimate_file_tokens;actualis the tokenised output of_read_files— the prompt that really gets sent. The only difference between the rows is the container, and the estimate moves by 4x._pack_chunks_by_tokenssizes chunks from that number, so a 3.45x undercount overfills the chunk, the request blows the context window, and_extract_with_adaptive_retrybisects — paying for the same content two, four or eight times before it fits. On a corpus of papers, which is what a PDF implies.Nothing looks broken. No wrong output, no error the user sees; the run just costs several times what it should. The bisection machinery works exactly as designed and quietly absorbs the mis-estimate, which is why this can sit there indefinitely.
After the fix, both encodings estimate 4582 against 4598/4599 actual — 1.00x, the remaining gap being the wrapper-overhead constant.
Scope
PDFs only. Every other path still measures its own bytes, because for them
read_textis what the prompt carries.The tokenizer-absent fallback gets the same correction: it used
stat().st_size, which for a PDF is the compressed size and is wrong in the same direction, so the fallback no longer disagrees with the tiktoken path about what a PDF costs.The memo
Estimating a PDF now means extracting its text, and packing probes the same file repeatedly while deciding where a chunk ends — so
_pdf_text_for_estimatememoises on(path, size, mtime_ns). A corpus of papers is parsed once per run rather than once per probe.Keyed on size and mtime rather than path alone so a file rewritten mid-run is re-read instead of served a stale estimate (there is a test for that), and bounded so a large corpus cannot pin every paper's text in memory.
Tests
tests/test_pdf_token_estimate.py(10 tests). The fixtures build real one-page PDFs with a genuine text layer, one raw and one FlateDecode, and first assert the precondition — that both hold identical text and the compressed one really is a fraction of the size — so any estimate gap is the bug rather than the fixture.Then: the estimate tracks the prompt it will build for both encodings, compression does not change the estimate (the tell — it used to differ by 4x purely because one was compressed), the estimate is not derived from file size, repeated probes agree, a rewritten PDF is re-estimated, a corrupt PDF estimates 0 rather than raising, non-PDF estimates are untouched, and the
_FILE_CHAR_CAPtruncation still bounds the result.Reverting
llm.pyand keeping the tests fails 3 of 10.One of those tests caught me while writing it: my first version grew the fixture from 400 to 800 lines to prove the memo re-reads, and it failed — both sizes are past
_FILE_CHAR_CAP, so they legitimately estimate the same and the test proved nothing. It now stays under the cap on purpose, and says so.Validation
Windows 11, Python 3.12, branched off
b14b52e(0.9.47).15 failed, 4738 passed->15 failed, 4748 passed. Identical failure set — no regressions; the +10 are the new tests.Related
Same class as #2900/#2902: the estimate and slicing paths read the raw file while the prompt path goes through
_file_to_text. That PR notes PDFs still need the converter threaded throughread_slice_text; this fixes the independent half in token estimation, which costs money even where slicing never applies. The two do not overlap in code.