Repository navigation
LW-474 [FEAT] include_cell_borders - #40
Open
johnyrahul wants to merge 3 commits into
Open
johnyrahul wants to merge 3 commits into
johnyrahul wants to merge 3 commits into
Conversation
LLMWhisperer draws an Excel workbook's own cell borders into the extracted text as box-drawing characters, and those cost tokens downstream: a small bordered sheet measures 273 characters with them and 113 without. include_cell_borders=false drops every border while keeping columns aligned and every cell value intact. The parameter keys off the uploaded file type, not the mode -- a spreadsheet routes to the Excel extractor under the default form mode as well as under excel, which is how most Excel uploads actually arrive -- and the service ignores it for every other kind of document. It is sent only when the caller sets it, like the other caller-only parameters above it: sending a default would pin a value the service would otherwise choose, and omitting it keeps this client working against deployments that predate the parameter. The spec hunk was produced by running upstream's own tools/gen_spec.py at the head of Zipstack/unstract-llm-whisperer#810, which adds the parameter to the controller but has not regenerated its spec yet. Re-sync byte-for-byte once that lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
johnyrahul
marked this pull request as ready for review
October 26, 2026 09:40
johnyrahul
requested review from
arun-venkataswamy,
jaseemjaskp,
muhammad-ali-e and
ritwik-g
October 26, 2026 09:40
#810 has since regenerated its own spec, and what it committed is byte-for-byte what running its generator at the earlier head produced -- so the vendored spec and the generated transport are both unchanged here. Only the recorded provenance moves, from the pre-regeneration head to db4cb15, and the note explaining that work-around goes with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
 [Medium impact] The changes since the previous review appear safe to merge.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
#810 was squash-merged, so the PR head recorded before it landed is not on main and will not be found by anyone retracing where this spec came from. The squash commit is, and its spec is identical to what is vendored here, so the spec and the generated transport are both unchanged -- only the recorded revision moves, and the note about the PR being unmerged goes with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
greptile-apps
Bot
dismissed
their stale review
October 26, 2026 09:53
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Contributor
|
This branch has not been deployed
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
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
Adds
include_cell_borderstoLLMWhispererClientV2.whisper().include_cell_borders=Falsestops LLMWhisperer drawing an Excel workbook's own cell borders into the extracted text as box-drawing characters (│ ─ ┌ ┐ └ ┘ ├ ┤ ┼). Columns stay aligned and no cell value is lost.It keys off the uploaded file type, not the mode. It applies to any XLS or XLSX upload whatever mode was requested — including the default
formmode, which is how most Excel uploads actually arrive, since a spreadsheet is routed to the Excel extractor underformas well as underexcel. It is not an "excel mode" parameter, and the service ignores it for every other kind of document. (Upstream's_resolve_cell_bordersgates onAllowedFileTypes.XLS/XLSXand returns themark_*_linespair untouched for anything else.)Why
Those border characters cost real tokens in whatever LLM consumes the output — a small bordered sheet measures 273 characters with its borders and 113 without. Customer-facing request via Vikash.
How
The usual order for this repo: spec → regenerate → expose in the facade.
specs/llmwhisperer.json— the parameter (boolean,default: true, notx-internal), byte-for-byte what is on upstreammain.SPEC_SOURCE_REVmoves to9fe7085, the squash commit of #810.src/unstract/llmwhisperer/_sdk_llmwhisperer/api/whisper/extract.py— regenerated by./tools/gen_sdk.sh, not hand-edited. The generated typed_get_kwargsis why the spec has to carry the parameter first: the facade calls it by keyword, so a parameter missing from the spec cannot be passed at all.client_v2.py— added to the_SEND_ONLY["extract"]declared set (without which it is never sent) and to the keyword-only signature.The default-sending decision: omitted unless the caller sets it
include_cell_borders: bool | None = None, added to the conditionalparams.update(...)block rather than the always-sentparamsdict. Nothing goes on the wire unless the caller passes it.This is not a new judgement — it is what the existing caller-only parameters (
allow_rotated_text,ignore_vertical_text,min_table_width, …) already do, and that block's own comment gives the reason:It also means this client keeps working unchanged against every deployment that predates the parameter, which still matters while the backend change rolls out. The older always-sent booleans (
mark_vertical_lines,mark_horizontal_lines) only look inconsistent — they mirror a service default offalseand have always been sent; this one's service default istrue, so sending it would be pinning.✅ Backend prerequisite has shipped
Zipstack/unstract-llm-whisperer#810 merged (squash commit
9fe7085), so the earlier "do not merge before the backend" hold on this PR is cleared.The spec provenance is now ordinary. For the record, since earlier revisions of this PR said otherwise: when this was opened, #810 had added the parameter to its controller but had not regenerated its own spec — its
spec-driftgate was red for exactly that reason — so there was no upstream revision whose committed spec carried the parameter. Rather than hand-write the hunk, it was produced by running upstream's owntools/gen_spec.pyat #810's then-head. #810 has since regenerated and landed, and what is onmainis byte-for-byte identical to what that produced (verified by diff), so the vendored spec and the generated transport never needed to change. #810 was squash-merged, soSPEC_SOURCE_REVpoints at the squash commit that is actually onmainrather than at a PR head that is not.One small thing worth raising upstream, not a blocker: the parameter has no
descriptionin the spec, because descriptions come from upstream'sspecs/overlay/llmwhisperer.yamland #810 adds no entry there. That is what would land in the generated transport's docstring. The facade docstring in this PR is written out in full regardless.Notes on Testing
uv run --group test pytest tests/unit— 293 passed, includingcompat_test.py(parity against the released baseline).tests/unit/client_v2_test.py: absent from the query string when unset, and present asFalse/Truewhen set either way.ruff check,ruff format,mypy .anddocformatterall clean. (pre-commit runcould not bootstrap its isolated hook env locally — a uv-python venv issue, unrelated to this change — so the configured linters were run directly.)include_cell_bordersagainst a live API. Worth adding once the backend change is deployed to the environment the integration tier points at; it was not assertable when this PR was written.Version
__version__is deliberately not bumped — per thespec-upgradeskill, the in-repo value is the last released version andmain.ymlapplies the bump at dispatch time; bumping it in a PR makes the release skip a version. Release this as a minor: new caller-visible behaviour, no signature break (a new parameter with a default, so theapi-surfacegriffe gate is unaffected).Relevant Docs
llmwhisperer-docs(this repo's README points at the docs site and deliberately does not enumerate whisper parameters, so there is nothing to change here).Related Issues or PRs
llmwhisperer-js-client) is not in scope here.🤖 Generated with Claude Code