fix: handle empty batches in detection and end-to-end predictors - #2138
linhongyu510 wants to merge 2 commits into
Conversation
7d8556e to
a11910c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2138 +/- ##
==========================================
- Coverage 97.23% 97.21% -0.02%
==========================================
Files 169 169
Lines 10039 10045 +6
==========================================
+ Hits 9761 9765 +4
- Misses 278 280 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
felixdittrich92
left a comment
There was a problem hiding this comment.
Hi @linhongyu510 👋,
Thanks for the PR. Left one comment.
| def test_predictors_on_empty_batch(mock_vocab): | ||
| """An empty page list must yield an empty Document instead of raising. | ||
|
|
||
| Filtering a batch down to nothing is ordinary caller code, and |
There was a problem hiding this comment.
Please remove the docstring afterwards fine to merge 👍
Passing an empty page list crashed three levels deep instead of returning an empty result. `RecognitionPredictor` has always short-circuited on an empty input, and `OrientationPredictor` gained the same behaviour in mindee#2069, but the detection and end-to-end predictors did not, so filtering a batch down to nothing raised from internals that never mention the empty input: - `PreProcessor.batch_inputs` computes `num_batches == 0` correctly, then reads `samples[0]` to pick the tuple/tensor branch -> `IndexError` - `detach_scores` calls `zip(*(...))` over no boxes -> `ValueError: not enough values to unpack (expected 2, got 0)` - on the KIE path `invert_data_structure` reads `x[0]` -> `IndexError` Guard at the three public entry points rather than patching each internal, so the existing helpers keep their non-empty precondition. `DetectionPredictor` returns the shape its `return_maps` contract promises, and the end-to-end predictors return an empty document of their own type -- `Document` for `OCRPredictor`, `KIEDocument` for `KIEPredictor`, whose per-class page shape would otherwise be lost to the base class. Verified `ruff check`, `ruff format --check` and `mypy doctr/` clean; reverting the guards turns the new test red at `preprocessor/pytorch.py:73`. Co-authored-by: Claude <noreply@anthropic.com>
Requested in review. The repository convention is no docstring on test functions (3 of 371 have one, and both others are single-line), and the inline comments already carry the load-bearing context.
a11910c to
05d0615
Compare
|
Docstring removed in Also rebased onto current Re-verified after the rebase rather than assuming the earlier run still held: The 6 errors are fixture downloads timing out on this machine ( I also re-confirmed the test still earns its place after the edit, by reverting each guard one at a time:
Three distinct failure points, matching the three in the description. All guards restored afterwards. |
|
One thing that needs your click: the nine workflows on
So CI here needs an "Approve and run" whenever I push. Since the only change between the two heads is the 10-line docstring deletion plus the rebase onto |
|
The docstring is gone and CI is green now that you approved the run — 56 of the 57 checks pass, including The one red check is Codecov's own per-file data says so. Of the 169 files, exactly one has a changed miss count, and it is not one of mine:
For that last file Codecov reports Root cause. _params = np.random.rand(1)
quad_idx = int(_params[0] / 0.25) # 0,1,2,3 -> four different branches
if quad_idx % 2 == 0: # line 173
...
if quad_idx == 0: ... # line 175
else: # line 178
...
if quad_idx == 1: ... # line 180Each run reaches only some of those lines, so the miss count for the file moves on its own. Measured with Over 400 seeds the four quadrants come up 104 / 99 / 95 / 102 times, so which lines get counted is close to a coin flip per run. This also shows up on
A docs-only commit moved it by the same 2 misses / 0.02%. My own earlier head So this PR adds 7 executable lines, all 7 covered, and touches no other file. Happy to seed the RNG in the shadow-mask tests as a separate PR if you'd like that flake fixed — it is unrelated to this change, so I did not fold it in here. One request: the review is still marked |
|
Opened #2144 for the coverage flake described above — it is independent of this PR and touches only If #2144 goes in first, the |
|
@felixdittrich92 Two small maintainer actions would unblock this pair — no code change needed on either. Here (#2138): the docstring you asked to remove is gone ( And #2144, the fix for that flake, currently shows only 1 check run — the fork-PR approval gate has not been triggered on it at all, so there is no result to look at. An "Approve and run" would let it report. It is test-only, Worth noting the two are order-independent: if #2144 lands first, the |
DetectionPredictor,OCRPredictorandKIEPredictorraise on an empty page list instead of returning an empty result.RecognitionPredictorhas always short-circuited on empty input (recognition/predictor/pytorch.py:50), andOrientationPredictorgained the same behaviour in #2069 — these three were the remaining gap, so this follows that precedent rather than introducing a new convention.Filtering a batch down to nothing is ordinary caller code (skip already-processed pages, drop files that failed a check upstream), and today it fails from internals that never mention the empty input.
Reproduction
There are three separate failure points on the way down, which is why the fix is not a one-liner in a single helper:
preprocessor/pytorch.py:73num_batchesis correctly0, thensamples[0]is read to pick the tuple/tensor branch →IndexErrorutils/geometry.py:124zip(*(_detach(box) for box in boxes))over no boxes →ValueError: not enough values to unpack (expected 2, got 0)models/_utils.py:276{k: ... for k in x[0]}→IndexErrorI confirmed the ordering by fixing them one at a time: guarding
batch_inputsmoved the crash todetach_scores, and guarding that one let the whole call returnDocument(pages=[]).Fix
Guard at the three public entry points instead of patching each internal, so
batch_inputs,detach_scoresandinvert_data_structurekeep their non-empty precondition and stay simple.Two details worth flagging:
DetectionPredictorreturns the shape its ownreturn_mapscontract promises:[]normally,([], [])whenreturn_maps=True.KIEPredictorreturnsKIEDocument(pages=[]), notDocument(pages=[]).KIEDocumentsubclassesDocument, so returning the base class would type-check and pass anisinstanceassertion while silently dropping the per-class page shape. The test asserts the exact type for this reason — I verified it catches the degradation by deliberately returningDocumentthere and watching the test go red.Tests
Added
test_predictors_on_empty_batch, which covers detection (bothreturn_mapsvalues), recognition (already-correct, asserted so the three stay consistent), and both end-to-end predictors.The test is load-bearing: reverting the guards fails it at
preprocessor/pytorch.py:73.The errors and the one failure are pre-existing on this machine and unrelated: the errors are
requests.exceptions.ConnectionErrorfrom tests that download weights or fixtures, andtests/common/test_io.py::test_read_htmlfails withOSError: cannot load library(missing WeasyPrint system libs). I verified thetest_read_htmlfailure reproduces on a clean checkout viagit stash.AI assistance was used for this change; I reviewed every changed line and ran the commands above locally.