Skip to content

Commit 487c0ad

Browse files
committed
LCORE-1471 spike: address PR lightspeed-core#1598 review feedback
Apply CodeRabbit's actionable comments and the per-comment nits: 1. PoC results section in spike doc previously listed paths under poc-results/ that are deleted before merge per howto-run-a-spike.md step 10, leaving broken links in the merged document. Replace the file list with a self-contained summary of what the PoC proved plus the heading-degradation finding, and a note pointing future readers at the PR diff if the raw artifacts are ever needed. 2. Drop the reference to docs/local-stack-testing.md (a local-only file, never committed to the repo). 3. Replace fragile line-numbered references (document_processor.py:75, :87, byok_guide.md ~106-118) with stable symbol anchors: _BaseDB.__init__, _LlamaStackDB.__init__, "Knowledge Sources" subsection, "Step 1" subsection. Line numbers rot; section names and symbol names rot less. 4. Spec doc now instructs the implementation ticket to extract the ("markdown", "html", "pdf") predicate to a single MARKDOWN_COMPATIBLE_DOC_TYPES: Final[tuple[str, ...]] constant in document_processor.py and reference it from both call sites, instead of duplicating the tuple. JIRA #1 scope updated to match. 5. Add R7: PDFReader.load_data emits a logger.warning when its docling output is empty / under a small threshold (a likely indicator of a scanned PDF given R5's no-OCR scope). Threshold is a module-level Final[int] constant. JIRA #1 scope and JIRA lightspeed-core#2 test patterns updated to require coverage via caplog. Surfacing the silent- degradation case in custom_processor.py logs costs nothing and makes the OCR-needed signal visible. Plus the two reviewer nits worth carrying into JIRA #1: - Use docling's TableFormerMode.ACCURATE enum, not the string literal "accurate"; both work via Pydantic coercion but the enum is type-checked. - Mirror HTMLReader's choice on whether to call super().__init__(); llama-index's BaseReader does not require it but symmetry between the two readers is preferred. The spec doc changelog records this revision and its trigger (the PR lightspeed-core#1598 CodeRabbit review).
1 parent 250881e commit 487c0ad

2 files changed

Lines changed: 57 additions & 45 deletions

File tree

docs/design/byok-pdf/byok-pdf-spike.md

Lines changed: 23 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ These determine scope and approach.
4343
| B | All in `rag-content` (skip stack docs update) |
4444
| C | All in `lightspeed-stack` (move BYOK pipeline) |
4545

46-
**Recommendation**: **A**. The "existing tool for producing BYOK vector store" *is* `rag-content`. `lightspeed-stack/docs/byok_guide.md` line 106-118 currently tells customers "PDFs must be converted to markdown first" — that needs to change to reflect native support. Confidence: 95%.
46+
**Recommendation**: **A**. The "existing tool for producing BYOK vector store" *is* `rag-content`. `lightspeed-stack/docs/byok_guide.md` currently lists PDF under "Requires conversion" and tells customers PDFs must be converted to Markdown first — that needs to change to reflect native support. Confidence: 95%.
4747

4848
## Technical decisions for @maxrubyonrails
4949

@@ -70,7 +70,7 @@ After docling exports to Markdown, how is the content chunked into vector-store
7070

7171
| Option | Description |
7272
|--------|-------------|
73-
| A | Reuse the existing `MarkdownNodeParser`. Add `"pdf"` to the `doc_type` branches at `document_processor.py:75,87`. |
73+
| A | Reuse the existing `MarkdownNodeParser`. Add `"pdf"` to the `doc_type` branches in `_BaseDB.__init__` and `_LlamaStackDB.__init__` (extracted to a shared `MARKDOWN_COMPATIBLE_DOC_TYPES` constant — see spec doc). |
7474
| B | Use docling's hybrid chunker (PDF-aware, page-boundary-aware). Different node parser path. |
7575

7676
**Recommendation**: **A**. docling already exports clean Markdown for body content (PoC evidence: tables, lists, paragraphs all preserved). The `MarkdownNodeParser` we use for HTML and Markdown is well-tested and handles the output. **B** adds a parallel chunking pipeline and complicates `document_processor.py`. If retrieval quality on PDFs is poor in practice, **B** is a clean follow-up. Confidence: 85%.
@@ -108,10 +108,11 @@ Four sub-JIRAs under [LCORE-1471](https://issues.redhat.com/browse/LCORE-1471).
108108
**Scope**:
109109

110110
- New package `src/lightspeed_rag_content/pdf/` with `__init__.py`, `__main__.py`, `pdf_reader.py`.
111-
- `PDFReader(BaseReader)` exposing `load_data(file: Path) -> list[Document]`.
111+
- `PDFReader(BaseReader)` exposing `load_data(file: Path) -> list[Document]`. Use `TableFormerMode.ACCURATE` (enum), not the string `"accurate"`. Match `HTMLReader` on whether to invoke `super().__init__()`.
112112
- `convert_pdf_file_to_markdown` and `convert_pdf_string_to_markdown` convenience helpers (mirror HTML).
113113
- CLI subcommands `convert` and `batch` (mirror `html/__main__.py`).
114-
- Update `document_processor.py` line 75 and line 87: `doc_type in ("markdown", "html", "pdf")`.
114+
- Per **R7**, `PDFReader.load_data` emits a `logger.warning` when the resulting Markdown is empty / under a small threshold (likely a scanned PDF). Threshold is a module-level `Final[int]` constant.
115+
- Extract the `("markdown", "html", "pdf")` predicate in `document_processor.py` to a single module-level `MARKDOWN_COMPATIBLE_DOC_TYPES: Final[tuple[str, ...]]` constant and reference it from both `_BaseDB.__init__` and `_LlamaStackDB.__init__` (do not duplicate the tuple).
115116
- Update `rag-content/README.md`: list PDF as a directly supported input format.
116117
- Pass `uv run make format && uv run make verify` (or rag-content equivalent).
117118
- No new entries in `pyproject.toml` — docling is already there.
@@ -126,11 +127,12 @@ Four sub-JIRAs under [LCORE-1471](https://issues.redhat.com/browse/LCORE-1471).
126127
**Agentic tool instruction**:
127128

128129
```text
129-
Read the "Architecture" and "Implementation" sections in
130-
docs/design/byok-pdf/byok-pdf.md (in the lightspeed-stack repo).
130+
Read the "Architecture", "Chunking", and "Implementation Suggestions"
131+
sections in docs/design/byok-pdf/byok-pdf.md (in the lightspeed-stack
132+
repo). R7 (warn on empty output) is in the "Requirements" section.
131133
Key files in rag-content:
132-
src/lightspeed_rag_content/html/ (precedent — mirror this)
133-
src/lightspeed_rag_content/document_processor.py:75,87
134+
src/lightspeed_rag_content/html/ (precedent — mirror this)
135+
src/lightspeed_rag_content/document_processor.py (extract MARKDOWN_COMPATIBLE_DOC_TYPES, use it in _BaseDB.__init__ and _LlamaStackDB.__init__)
134136
README.md
135137
Mirror html/ into pdf/ with InputFormat.PDF and the pipeline options
136138
listed in the spec doc's "Pipeline configuration" section.
@@ -175,7 +177,6 @@ Use docling's mock-friendly seam from the HTML tests.
175177

176178
**Scope**:
177179

178-
- Reuse the local-stack-testing pattern from [docs/local-stack-testing.md](../../local-stack-testing.md).
179180
- Add an e2e feature file under `tests/e2e/features/` (BDD style).
180181
- Step definitions that (1) generate a vector store from a sample PDF, (2) start the stack pointed at it, (3) issue a query, (4) assert retrieved content matches PDF source.
181182
- Add the new feature to `tests/e2e/test_list.txt`.
@@ -190,7 +191,6 @@ Use docling's mock-friendly seam from the HTML tests.
190191
```text
191192
Read the "End-to-end validation" section in docs/design/byok-pdf/byok-pdf.md.
192193
Key files:
193-
docs/local-stack-testing.md
194194
tests/e2e/features/ (existing BDD features for pattern)
195195
tests/integration/endpoints/test_query_byok_integration.py (similar pattern)
196196
Generate the vector store ahead of stack startup using the rag-content
@@ -206,9 +206,8 @@ custom_processor.py invocation documented in the spec doc.
206206
**Scope**:
207207

208208
- Edit `docs/byok_guide.md`:
209-
- Line ~106 (`Directly supported`): add PDF.
210-
- Line ~107 (`Requires conversion`): remove PDF; clarify which formats still require conversion.
211-
- Line ~114-118 (Step 1): remove the docling-as-pre-conversion example, replace with a note that PDFs can be passed directly to `custom_processor.py`.
209+
- "Knowledge Sources" subsection (under Prerequisites): move PDF from "Requires conversion" to "Directly supported"; clarify which formats still require conversion (e.g., AsciiDoc).
210+
- "Step 1: Prepare Your Knowledge Sources": remove the docling-as-pre-conversion example for PDFs, replace with a note that PDFs can be passed directly to `custom_processor.py`.
212211
- Sanity-check no other parts of `docs/` give stale conversion advice (search for `docling` and `convert.*PDF`).
213212

214213
**Acceptance criteria**:
@@ -221,7 +220,8 @@ custom_processor.py invocation documented in the spec doc.
221220
```text
222221
Read the "Documentation impact" section in docs/design/byok-pdf/byok-pdf.md.
223222
Key files:
224-
docs/byok_guide.md (lines 106-118)
223+
docs/byok_guide.md (Knowledge Sources subsection
224+
+ Step 1)
225225
examples/lightspeed-stack-byok-okp-rag.yaml (no change, but verify still
226226
accurate after PDF support)
227227
```
@@ -249,21 +249,16 @@ The PoC mirrors the production `HTMLReader` but configures docling for PDF (`Inp
249249
| sample_jira_1311.pdf | 217 KB | 332 s (incl. ~290 s model load) | 7,608 chars / 288 lines | High — clean headings, body, tables |
250250
| sample_jira_836.pdf | 372 KB | ~70 s (warm) | 3,084 chars / 165 lines | Body clean; **headings degraded** (letter-spaced font) |
251251

252-
Detailed findings, log excerpts, and converted Markdown are in [`poc-results/`](poc-results/):
252+
**What the PoC proved (validated against both samples)**:
253253

254-
- [`01-poc-report.txt`](poc-results/01-poc-report.txt) — methodology, findings, implications
255-
- [`02-conversion-log.txt`](poc-results/02-conversion-log.txt) — exact commands and timings
256-
- [`03-sample-jira-1311.md`](poc-results/03-sample-jira-1311.md) — converted output (clean)
257-
- [`04-sample-jira-836.md`](poc-results/04-sample-jira-836.md) — converted output (heading degradation visible)
254+
1. **No new dependencies are needed.** docling, already in `pyproject.toml` for HTML, handles PDF via the same `DocumentConverter` API with `InputFormat.PDF` and `PdfFormatOption(pipeline_options=...)`.
255+
2. **Body text and tables convert cleanly.** The 1311 sample produced two well-formed GitHub-flavored Markdown tables (Component/Behavior, Area/Impact) with correct headers and cell boundaries. Body paragraphs were free of hyphenation artifacts and broken sentences.
256+
3. **`MarkdownNodeParser` will work for chunking.** Heading prefixes (`## `, `### `) survive intact, so the existing chunker splits on heading boundaries without modification.
257+
4. **No parallel chunking pipeline needed.** A single `doc_type` branch addition is sufficient.
258258

259-
**Key takeaways the PoC proved**:
259+
**Honest limitation surfaced by sample 836** (Confluence "Export to PDF"): letter-spaced display fonts cause docling to extract heading text with spaces between letters (e.g., `S t r e a m l i n e l i g h t s p e e d - s t a c k c o n f i g`). The `## ` prefix remains intact so chunking still happens at heading boundaries; only the heading *text* is corrupted. Body content under the heading is unaffected. This is a docling extraction limitation, not a `PdfPipelineOptions` knob. Documented as a known v1 caveat in the spec doc; no production fix in v1. A heading-cleanup post-processor is noted as a follow-up.
260260

261-
1. No new dependencies are needed (docling already covers PDF).
262-
2. Recommended pipeline defaults produce clean Markdown for body content.
263-
3. Tables are preserved as proper Markdown tables.
264-
4. `MarkdownNodeParser` will work for chunking — no parallel pipeline needed.
265-
266-
**Honest limitation surfaced**: PDFs with letter-spaced display fonts (typical of Confluence "Export to PDF" output) produce noisy headings. This is a docling extraction limitation, not something `PdfPipelineOptions` controls. Document as a known caveat in the spec doc; no production fix in v1.
261+
> The full PoC report, conversion logs, sample inputs, and converted outputs were committed under `poc/` and `poc-results/` for review purposes. Per the spike workflow ([`howto-run-a-spike.md`](../../contributing/howto-run-a-spike.md) step 10), those artifacts are removed before merge; their content is summarised above so the spike doc stays self-contained in the merged history. The PR diff at the time of review (PR #1598) is the canonical reference if the raw artifacts are needed later.
267262
268263
## Background sections
269264

@@ -286,15 +281,14 @@ rag-content/
286281
└── html_reader.py # 165 LoC, BaseReader, uses docling
287282
```
288283

289-
The `document_processor.py` file already routes by `doc_type`:
284+
The `document_processor.py` file already routes by `doc_type` inside both `_BaseDB.__init__` and `_LlamaStackDB.__init__`:
290285

291286
```python
292-
# src/lightspeed_rag_content/document_processor.py:75
293287
if config.doc_type in ("markdown", "html"):
294288
Settings.node_parser = MarkdownNodeParser()
295289
```
296290

297-
This is the only line that needs to grow `"pdf"` for chunking to work.
291+
These are the only two predicates that need to grow `"pdf"` for chunking to work. The implementation ticket extracts the tuple to a shared `MARKDOWN_COMPATIBLE_DOC_TYPES` constant so the predicate cannot drift between the two call sites.
298292

299293
### HTML precedent (LCORE-1035)
300294

0 commit comments

Comments
 (0)