Skip to content

fix(parser/mineru): log items the text fallback drops without a trace - #3789

Open
boeschbenjamin-jpg wants to merge 1 commit into
HKUDS:mainfrom
boeschbenjamin-jpg:fix/mineru-silent-drop-log
Open

fix(parser/mineru): log items the text fallback drops without a trace#3789
boeschbenjamin-jpg wants to merge 1 commit into
HKUDS:mainfrom
boeschbenjamin-jpg:fix/mineru-silent-drop-log

Conversation

@boeschbenjamin-jpg

Copy link
Copy Markdown

Follow-up to the review of #3775 (Finding 2), kept separate so that PR stays small.

Problem

The text fallback in lightrag/parser/external/mineru/ir_builder.py drops any content_list item whose _coerce_text is empty without a single log line. That is exactly why the silently dropped chart items in #3774 stayed invisible until a 342-page book was audited by hand — an empty table body at least gets a logger.debug. The next picture-like type MinerU adds (or a v2-shaped payload nesting the path under content.image_source.path) would disappear the same way.

Fix

One elif in the fallback: emit a logger.debug breadcrumb with type and page_idx for items that carry no usable text and are not known-empty-by-nature.

_KNOWN_EMPTY_TYPES holds the text-typed items whose emptiness is ordinary layout noise (blank running head, a heading the model could not read) — logging those would be per-page spam. The types with their own dispatch branch (text / list / code / equation / table / the drawing types) and page_number never reach the fallback, so they are not listed.

No behaviour change beyond the log line: the item is still skipped, no position is recorded.

Test

test_adapter_logs_structural_item_dropped_without_text — an unhandled picture-like item (header_image with img_path, no text) produces exactly one message; a blank header on the same page stays silent. Fails without the change (assert set() == {...}).

$ pytest tests/parser/external/mineru/test_ir_builder.py tests/parser/external/mineru/test_parse_mineru_sidecar.py -q
38 passed, 2 warnings in 0.26s

🤖 Generated with Claude Code

https://claude.ai/code/session_01GqSL71EiBLCnvv7VnvKCDj

The fallback discards any content_list item whose `_coerce_text` is empty
without a single log line. That is why the dropped `chart` items (HKUDS#3774)
stayed invisible until a 342-page book was audited by hand, and the next
picture-like type MinerU adds — or a v2-shaped payload that nests the path
under `content.image_source.path` — would vanish the same way.

Emit a debug breadcrumb for those, but stay silent for text-typed items
whose emptiness is ordinary layout noise (blank running head, unreadable
heading); `_KNOWN_EMPTY_TYPES` lists them. The types with their own branch
(text/list/code/equation/table/drawings, page_number) never reach here.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GqSL71EiBLCnvv7VnvKCDj
@danielaskdd

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-01T15:02:55.390784Z a93d70c Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown
Contributor

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: a93d70cf38

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@danielaskdd

Copy link
Copy Markdown
Collaborator

Review of a93d70c

The change is correct and has no behavioural side effect. One requested change before merge, plus two smaller items.

Verified independently

  • tests/parser/external/mineru103 passed at this head; reverting ir_builder.py to main turns test_adapter_logs_structural_item_dropped_without_text red (assert set() == {...}), so the test discriminates. The propagate toggle in _captured_logs is restored in a finally, and the set-based dedup comment is accurate.
  • _KNOWN_EMPTY_TYPES checked type by type: text / list / code / equation / table / the drawing types / page_number all continue out of their own branch and genuinely never reach the fallback; title / section_header reach it only when empty (_detect_heading returns a heading for any non-empty one), which is exactly why they have to be on the list. The reasoning in the comment block holds.
  • _append_text returns False only for falsy text, so "no usable text" cannot misreport an item that did contribute.
  • Merged with fix(parser/mineru): keep MinerU chart items instead of dropping them #3775 locally: ort auto-merges with no conflict, and after both land chart has its own branch and no longer reaches the fallback. Either merge order works.
  • ruff check / ruff format --check clean; CI 4/4 green.

Requested change — a DEBUG line does not surface this

The PR's own claim is "A one-liner would have surfaced this in the first ingest". It would not: deployments run at INFO, so the operator who lost 5 charts in #3774 still has to suspect something first, then re-run the whole 342-page book with DEBUG on. The breadcrumb is the right detail; what is missing is the signal.

Please add an end-of-parse aggregate at WARNING, keeping the per-item debug as-is. I prototyped it on your branch — this exact patch, ruff format-clean:

        dropped_by_type: dict[str, int] = {}
        for item_index, item in enumerate(content_list):
            ...
            elif item_type not in _KNOWN_EMPTY_TYPES:
                logger.debug(...)
                dropped_by_type[item_type] = dropped_by_type.get(item_type, 0) + 1

        _flush_block()

        if dropped_by_type:
            logger.warning(
                "[mineru_ir_builder] %d content_list item(s) dropped with no "
                "usable text: %s",
                sum(dropped_by_type.values()),
                ", ".join(
                    f"{t or '<untyped>'}={n}"
                    for t, n in sorted(dropped_by_type.items())
                ),
            )

Against a bundle holding an unhandled header_image, two chart items, a blank header and one item with no type at all, at WARNING level:

WARNING: [mineru_ir_builder] 4 content_list item(s) dropped with no usable text: <untyped>=1, chart=2, header_image=1

One line per document, silent when nothing is dropped, and it names the type an operator would then go looking for. <untyped> covers the item whose type / label are both absent, which item_type renders as the empty string.

This does raise the bar above the existing _build_ir_table empty-table logger.debug (ir_builder.py:459) — that is intended: an empty table is a local drop the operator can see in the output, while an unmapped item type is a whole class of content silently missing from the document.

Also worth folding in — the breadcrumb is missing what you would need

type + page_idx (which can be None) is not enough to diagnose the case the PR is aimed at, "a payload shape the dispatch does not know yet". The item's index and its key set are the diagnostic part, and neither carries document content:

logger.debug(
    "[mineru_ir_builder] dropping item with no usable text "
    "(type=%s, page_idx=%s, self_ref=%s, keys=%s)",
    item_type,
    item.get("page_idx"),
    _content_list_self_ref(item_index),
    sorted(item),
)

Note this changes the message the new test asserts verbatim — the expected string needs updating along with it (that was the only failure when I ran the suite against my prototype: 102 passed, 1 failed on the stale expected string).

Optional, same class of hole

An empty equation is still dropped by a bare continue (ir_builder.py:331-ish) with no log at all, while an empty table gets its debug line. If you would rather keep this PR tight, a follow-up is fine — just flagging it since it is the same silent-drop shape.

Happy to merge once the aggregate warning is in.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants