Add read-only production runtime audit
This commit is contained in:
@@ -0,0 +1,216 @@
|
||||
# ADR 0005: `segment/` output contract needed by `chunk/` — structure-preserving, not flattened
|
||||
|
||||
## Status
|
||||
|
||||
Proposed. **Contract/schema only — no implementation.** `segment/models.py`,
|
||||
`segment/assembler.py`, and `segment/io.py` are actively owned by a parallel
|
||||
session on the same checkout at the time of writing; this ADR specifies what
|
||||
`chunk/` needs from `segment/`'s output precisely enough to implement and
|
||||
test, but does not touch those files itself. Supersedes part of ADR 0004
|
||||
(see "Relationship to ADR 0004" below) — ADR 0004's `(drug_id, section_key)`
|
||||
chunk-unit-as-leaf assumption is corrected here to `(drug_id, section_key)`
|
||||
as a **parent**, with sentence-window splitting demoted from primary
|
||||
strategy to fallback.
|
||||
|
||||
## Context
|
||||
|
||||
ADR 0004 designed chunking against `segment/models.py`'s current output:
|
||||
`SectionSpan.text` is a single flattened string per section (`"\n".join(
|
||||
body_line.strip() for ...)`), with all per-line style (`Span.bold`) and
|
||||
per-line page position discarded once the string is built (confirmed by
|
||||
reading `assembler.py`: `body_lines.append(span.text.strip())` keeps only
|
||||
`span.text`, nothing else). Review of ADR 0004 surfaced four real problems
|
||||
that trace back to this flattening, not to the chunking algorithm itself:
|
||||
|
||||
1. **A section is not a single semantic unit.** `liều lượng và cách dùng`
|
||||
and `tương tác thuốc` routinely contain multiple distinct facts (dosing
|
||||
per patient population, dosing per organ-function impairment, multiple
|
||||
separate drug interactions) that a doctor may want to retrieve
|
||||
independently. Measured: an explicit population marker ("Người lớn"/
|
||||
"Trẻ em"/"Trẻ sơ sinh"/"Suy thận"/"Suy gan" immediately followed by `:`
|
||||
or `.`) appears in **303 of 675 monographs (44.9%)** that have a `liều
|
||||
lượng và cách dùng` section — this is common, not an edge case.
|
||||
2. **A blind sentence-boundary sliding window (ADR 0004's original
|
||||
sub-chunking method) can still split two different facts into the same
|
||||
chunk, or split one fact across two chunks**, because it has no way to
|
||||
know a population/interaction boundary exists — that information exists
|
||||
in the source (as a bold or otherwise visually distinct sub-heading, per
|
||||
direct reading of MORPHIN SULFAT/VITAMIN D section text: lines like
|
||||
"Thuốc uống", "Cách dùng:" render as isolated bold short lines in the
|
||||
PDF) but is discarded before `chunk/` ever sees it.
|
||||
3. **Tables inside the monograph range are not addressed at all.** ADR 0004
|
||||
implicitly assumed monograph-range sections are prose. `docs/pdf-parsing-
|
||||
outlier-catalog.md` item 19 already documents a real table (dosing by
|
||||
renal function, HSV/CMV columns) inside a monograph body (Foscarnet
|
||||
natri, physical page 698) — flattening a table's rows into
|
||||
newline-joined body text destroys its row/column structure exactly the
|
||||
way outlier item 7 already describes for the appendix's 2D nomogram
|
||||
table. A whole-range survey to size this properly is in progress
|
||||
alongside this ADR (see "Not yet resolved" below).
|
||||
4. **Provenance is section-level, not chunk-level**, because per-line
|
||||
`physical_page`/`y0` (which `Span` already carries — see
|
||||
`extract/models.py`) is discarded at the same flattening point. For a
|
||||
section spanning several physical pages, a sub-chunk built from its
|
||||
final third currently has no way to know its own real page — it can
|
||||
only inherit the whole monograph's `source_page_range`. For medical
|
||||
citations this is not precise enough.
|
||||
|
||||
**A fifth, independently-found data-quality bug makes precise provenance
|
||||
even more necessary, not less**: the corpus's last-processed monograph
|
||||
(ZOLPIDEM) is never closed until true end-of-stream, and `assembler._classify`
|
||||
calls `match_section()`/`match_section_with_inline_value()` on every span
|
||||
with **no `in_monograph_range` gate** (unlike `_TextEvent` handling, which
|
||||
does check it). A spurious bold-text match on physical page 1655 — deep in
|
||||
the back-of-book "Mục lục tra cứu" brand-name index, confirmed by reading
|
||||
that page directly — overwrote ZOLPIDEM's real `tương tác thuốc`
|
||||
`SectionSpan` with an empty one and corrupted its `source_page_range` to
|
||||
`[1492, 1655]`. This is real content loss (measured: exactly 1 monograph
|
||||
affected, the last one processed — every other monograph is closed on
|
||||
schedule by the next monograph title, which *is* range-gated). Flagged for
|
||||
the session that owns `extract`/`segment`, not fixed here.
|
||||
|
||||
## Decision
|
||||
|
||||
Extend `segment/models.py`'s `SectionSpan` with a structured, line-level
|
||||
representation, additive to (not replacing) the existing flat `text` field
|
||||
— `chunk/` becomes a real, structure-aware consumer instead of re-deriving
|
||||
structure from a flattened string via ad hoc regex.
|
||||
|
||||
### New/changed types (`segment/models.py`)
|
||||
|
||||
```python
|
||||
@dataclass(frozen=True)
|
||||
class BodyLine:
|
||||
text: str
|
||||
physical_page: int
|
||||
y0: float
|
||||
bold: bool # Span.bold, preserved instead of discarded
|
||||
|
||||
@dataclass
|
||||
class SectionSpan:
|
||||
key: str
|
||||
display_name: str
|
||||
heading: Heading
|
||||
text: str # UNCHANGED meaning, kept for
|
||||
# backward compat (see invariant below)
|
||||
lines: List[BodyLine] = field(default_factory=list) # NEW
|
||||
```
|
||||
|
||||
`lines` carries exactly the per-line signal `chunk/` needs to do its own
|
||||
job (population/subheading detection, precise page provenance) without
|
||||
`segment/` having to know anything about chunking — `segment/`'s
|
||||
responsibility stays "detect boundaries and preserve source structure," not
|
||||
"decide what a retrieval unit is" (Clean Architecture / SoC, per
|
||||
CLAUDE.md). Specifically, this is deliberately **not** a `is_subheading:
|
||||
bool` field computed by `segment/` — classifying "is this line a
|
||||
subheading a chunker should split on" is a chunking-time decision (what
|
||||
counts as a good split point can vary by strategy/eval results), not a
|
||||
segmentation-time one. `segment/` should stop discarding the raw signal
|
||||
(`bold`, `y0`, `physical_page`) it already has per span; it should not also
|
||||
start doing chunk-shaping judgment calls.
|
||||
|
||||
### Invariants
|
||||
|
||||
1. `text == "\n".join(l.text for l in lines).strip()` for every
|
||||
`SectionSpan`, for the lifetime of this contract — `lines` is a strictly
|
||||
additive refinement, never a divergent second source of truth. Any
|
||||
change to how body text is assembled (e.g. the boilerplate-stripping fix
|
||||
already applied by the other session) must update both fields from the
|
||||
same filtered span list, not `text` alone.
|
||||
2. `lines` is in reading order, matching the order `text`'s lines already
|
||||
implicitly have.
|
||||
3. Every `BodyLine.physical_page` satisfies `detector.in_monograph_range`
|
||||
for a `Span` on that page — i.e., **no line in any `SectionSpan.lines`
|
||||
may come from outside the monograph's real printed-page range**. This is
|
||||
the ZOLPIDEM bug's exact failure mode stated as an invariant: it was
|
||||
violated (a spurious section event was accepted from a fully
|
||||
out-of-range page precisely because no such check existed for section
|
||||
*events*, only for body *text* events). Enforcing this invariant closes
|
||||
that bug as a side effect, but the invariant is stated here as a
|
||||
contract requirement independent of any specific fix implementation.
|
||||
4. Every currently-open monograph must be finalized exactly once, at either
|
||||
(a) the next monograph title, or (b) true end-of-stream — with no third
|
||||
path (e.g., a stray out-of-range section match) able to silently mutate
|
||||
an already-"complete" monograph's sections after point (a) would
|
||||
otherwise have applied. (This is a restatement of invariant 3 from the
|
||||
monograph-lifecycle side, not a new requirement.)
|
||||
|
||||
### Migration impact
|
||||
|
||||
- **`segment/io.py`** (`_monograph_to_dict`/`_monograph_from_dict`,
|
||||
`write_monographs_jsonl`/`read_monographs_jsonl`): additive — serialize
|
||||
`lines` alongside the existing `text`/`heading` fields per section.
|
||||
Existing consumers reading only `text` (e.g. `segment/atc.py`'s
|
||||
`extract_atc_codes`, which regexes over `SectionSpan.text`) need no
|
||||
change, per invariant 1.
|
||||
- **`ingestion/data/processed/monographs.jsonl`**: schema grows a new
|
||||
optional-shaped field (`sections[key].lines`). No `schema_version` field
|
||||
currently exists in the serialized dict (checked `io.py` directly) —
|
||||
worth adding as part of this change, both for this migration and because
|
||||
`docs/architecture.md` already assumes "collection aliasing allows
|
||||
re-ingesting with a changed chunking strategy," which implies the
|
||||
ingestion output itself should be able to declare which schema shape it
|
||||
is.
|
||||
- **Existing 110 tests**: unaffected if invariant 1 holds — no assertion in
|
||||
the current suite inspects `lines` (it doesn't exist yet), and `text`'s
|
||||
value/semantics are unchanged.
|
||||
- **New tests required** (this ADR specifies them; implementation and the
|
||||
actual test code are not part of this ADR):
|
||||
1. Regression test reproducing the ZOLPIDEM failure shape: a synthetic
|
||||
span stream — last monograph's title and real sections, followed by
|
||||
spans whose `printed_page` is out of `in_monograph_range` but whose
|
||||
text matches a `vocab.py` section label — asserting the monograph
|
||||
closes with its real sections intact and the out-of-range spurious
|
||||
match is ignored, not accepted.
|
||||
2. `SectionSpan.lines` fixture test: using the real MORPHIN SULFAT
|
||||
boilerplate-fix fixture already in `tests/test_segment_assembler.py`,
|
||||
assert `lines` preserves the correct `bold`/`physical_page`/`y0` per
|
||||
retained line (and that stripped boilerplate lines are absent from
|
||||
`lines` too, not just from `text`).
|
||||
3. Round-trip test: `write_monographs_jsonl` → `read_monographs_jsonl`
|
||||
preserves `lines` exactly (dataclass equality per line).
|
||||
4. Whole-corpus invariant-1 check: for a real `cli run` output, assert
|
||||
`text == "\n".join(l.text for l in lines).strip()` holds for every
|
||||
section of every monograph, not a sample.
|
||||
|
||||
## Relationship to ADR 0004
|
||||
|
||||
ADR 0004's chunk-unit decision (`(drug_id, section_key)`) is **not**
|
||||
discarded — a section is still the natural *parent* grouping (matches how a
|
||||
clinician thinks, matches `Monograph.sections`). What changes: ADR 0004
|
||||
described a section as directly *the* chunk when under the 800-token
|
||||
ceiling, with sentence-window splitting as the fallback for oversized
|
||||
sections. Per the review above, splitting must instead **first** attempt to
|
||||
break at real structural boundaries available in `SectionSpan.lines` (a
|
||||
bold, short, isolated line — the same "subheading" shape already visually
|
||||
confirmed for route-of-administration/population sub-headers — or an
|
||||
explicit population/organ-function marker), with the sentence-window method
|
||||
demoted to a fallback for the remaining prose that has no such marker. The
|
||||
exact splitting algorithm (how a "subheading-shaped line" is defined
|
||||
precisely, in code) is a `chunk/`-side implementation detail *enabled* by
|
||||
this contract, not decided by it.
|
||||
|
||||
## Not yet resolved (explicitly out of scope for this ADR)
|
||||
|
||||
- **Table/formula content blocks.** A separate whole-monograph-range survey
|
||||
(pdfplumber `find_tables()` + PyMuPDF math-symbol scan, physical pages
|
||||
98-1494 excluding blank page 99 — the exact set `detector.
|
||||
in_monograph_range` accepts, not an assumed offset) is in progress at the
|
||||
time of writing, per explicit user instruction to measure before deciding
|
||||
a table/formula chunk-unit strategy. This ADR's `BodyLine`
|
||||
contract covers **text content only**; a table/formula region should
|
||||
*not* currently be flattened into `BodyLine`s (doing so would repeat
|
||||
exactly the "destroys row/column meaning" mistake outlier item 7 already
|
||||
documents) — but the precise `ContentBlock`/table-row/formula-unit shape
|
||||
is deferred to a follow-up revision of this ADR once the survey reports
|
||||
real numbers (how many monographs/sections affected, page-break
|
||||
continuation frequency, multi-tier headers, merged cells, footnotes).
|
||||
- **Paragraph-boundary detection** (grouping consecutive `BodyLine`s into a
|
||||
flowing paragraph vs. a new one) is left to `chunk/`, using the same
|
||||
kind of y-gap heuristic `segment/merge.py` already validates for
|
||||
multi-line title wraps (`_MAX_LINE_GAP_PT`) — `BodyLine.y0` is sufficient
|
||||
raw signal for `chunk/` to compute this itself; `segment/` does not need
|
||||
to pre-compute paragraph grouping.
|
||||
- **The actual `chunk/` splitting implementation** (subheading detector,
|
||||
population-marker regex, sentence-window fallback) is not part of this
|
||||
ADR — this ADR defines the data contract that implementation will consume.
|
||||
Reference in New Issue
Block a user