fix(validators): fail fast on 0-record manifests + validate NLP text content#303
Conversation
…content
Adversarial end-to-end MLM ingestion surfaced several input-hygiene gaps that
the pre-ingestion validator chain missed. This hardens them at the
unit/validator level so they're caught BEFORE the table is created — never as a
late, misleading backend-registration error.
MUST FIX — "0 ingestable records":
- New IngestableRecordsValidator (wired into the NLP factories) rejects a
header-only / empty CSV ("No data rows found in CSV") and, for file-bearing
categories, a manifest whose every referenced file is missing ("No referenced
data files could be found; nothing to ingest"). Extends the #250 zero-byte
guard (which only caught a totally-empty file; a header-only CSV slipped
through). Runs in validate_data before create_table, so no orphan empty table.
- base.py registration-failure messages no longer claim "its rows are already
in the database" when 0 rows were ingested — _rows_state_clause() phrases the
parenthetical truthfully for the actual inserted-row count.
SHOULD FIX — NLP text content:
- New TextContentValidator (gated on text/token classification + MLM) samples
the referenced text files and rejects binary / non-UTF-8 content (NUL byte or
undecodable bytes) and warns on empty/whitespace-only docs. Sampled + byte-
capped (incremental UTF-8 decode tolerates a multibyte char split at the cap).
NICE TO HAVE — within-CSV duplicates:
- DuplicateValidator now also WARNS when a filename appears more than once in
the incoming CSV (the existing check only compared against the existing
table). Warning only — repeated filenames may be intentional.
Shared: BaseValidator._match_column centralises the case-insensitive header
lookup the new validators and BIO validator need.
Tests: add coverage for header-only CSV -> fail fast; all-referenced-files-
missing -> fail fast (no orphan table); binary/non-UTF-8 -> rejected; empty/
whitespace -> warned; within-CSV duplicate -> warning; truthful 0-row
registration message; and regression that a valid dataset still passes. Full
suite green (1183 passed), coverage 96.7% (gate 95%).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
👋 Heads-up — Code review queue is at 32 / 30 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
Bugbot (Medium): IngestableRecordsValidator used a plain os.path.join + os.path.isfile, but ingestion resolves paths with file_transfer._safe_join under SRC_PATH (#239). An absolute or `..` manifest value could make the plain join "find" a file OUTSIDE sequences/ or texts/ while the transfer rejects or skips every row — so validation passed, the table was created, and 0 rows were ingested: exactly the zero-record failure this PR targets. Resolve both new validators (IngestableRecordsValidator, TextContentValidator) with _safe_join under SRC_PATH, exactly like the transfer. A value that escapes the dataset dir raises ValueError and is skipped (not counted as found / not inspected), so a traversal/absolute manifest can no longer mask the zero-record case or read content outside the dataset directory. Tests: add traversal/absolute-path cases to both validators. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
bugbot run |
Bugbot (Medium): _inspect decoded with the incremental decoder and final=False only, so a small file ending in a truncated/wrongly-encoded multibyte sequence left the trailing bytes unflushed in the decoder buffer and passed as valid UTF-8. Finalize the decode (final=True) when the read reached EOF — detected by the read returning fewer bytes than the cap — so truncated trailing bytes are flushed and raise. We still keep final=False when we stopped exactly at the byte cap, so a legitimate multibyte char split at the cap isn't a false positive. Add a regression test for a small file with a truncated trailing multibyte sequence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1c8f80d. Configure here.
|
bugbot run |
Bugbot (Medium): TextContentValidator strided-sampled the manifest filenames and THEN skipped the missing ones. When most rows referenced absent files but a few real files existed, the present files could fall outside the sample, so docs_checked stayed 0 and binary / invalid-UTF-8 files were never read — IngestableRecordsValidator had already passed on the one existing file. Existence-filter first (a cheap stat per row), then strided-sample the files that ACTUALLY exist. The expensive part (reading + decoding content) stays bounded to sample_size; docs_checked now reflects real files, so any existing binary file is inspected. Add a regression test (one real binary file amid many missing references). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 32b199e. Configure here.
…n + IOB2 transition warning (#313) * fix(validators): fail fast when a classification CSV is missing the label column Adversarial text-classification ingestion (dev ingestor:0.3.12) found a CSV with header `filename,extension` (no `label`), configured `label: label`, slipping past every validator: every record cleaned to label=None and the backend rejected each row with HTTP 400 {"label":["This field may not be null."]} — a late, confusing failure for a simple missing/mis-named manifest column. Root cause: LabelDiversityValidator reads the label column lazily and PASSES when it's absent (it explicitly defers the missing-column case to "DataValidator or the ingestor"), but the label column is stripped out of the schema before DataValidator sees it — so nothing actually caught it. Fix: add LabelColumnValidator, a preflight presence check that fails fast (before the table is created) with a clear, actionable message naming the columns that ARE present. Wired into the text_classification factory BEFORE the diversity check. Token classification already rejects a missing label column via BIOLabelValidator, so it is intentionally not wired there; object detection sources labels from XML annotations (not a CSV column), so it's excluded too. Tests: missing column (default + custom name) fails fast; present (case- insensitive) passes; DataFrame + non-CSV/unreadable inputs handled; factory wiring (text-clf includes it before diversity; token-clf excludes it). Full suite: 1199 passed, 1 xfailed, total coverage 97% (gate 95%). Follow-up to merged #303 (0-record + NLP text-content validators). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(validators): make _match_column whitespace-insensitive (bugbot #313) CSVIngestor strips header whitespace on read (`columns.str.strip()`), so a header like ` label ` ingests as `label`. `_match_column` lowercased but did not strip, so LabelColumnValidator would falsely reject a manifest like `id, label ` that ingests fine. Strip both sides of the comparison (matching CSVIngestor and LabelDiversityValidator._resolve_column), still returning the original column name so callers index the raw frame correctly. Regression test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(validators): warn on IOB2 transition anomalies in token-classification labels Adversarial token-classification ingestion (dev ingestor:0.3.12) found that an `I-<TYPE>` opening an entity (no preceding `B-<TYPE>`/`I-<TYPE>` of the same type) ingests cleanly — e.g. `I-PER O O O B-ORG`. BIOLabelValidator validated each tag's FORMAT but never the IOB2 SEQUENCE, despite advertising "BIO/IOB2". Add `_iob2_sequence_warnings`: detect entities that open with `I-` (orphan `I-` at the start, `I-` after `O`, or a type switch like `B-PER I-ORG`) and surface them as a WARNING — not a hard error. Such sequences are malformed under IOB2 but LEGAL under IOB1 (a chunk may open with `I-`), and the validator is scheme-agnostic, so hard-failing would wrongly reject valid IOB1 datasets. The warning lets a user who intended IOB2 catch the problem without blocking IOB1. Checked only on format-valid tags (no double-reporting with the invalid-tag error) and before file I/O (so it surfaces even when the .txt is missing). Tests: orphan-I warns but doesn't block; type-switch warns; well-formed has no warning; format-invalid suppresses the IOB2 warning. Full suite: 1204 passed, 1 xfailed, total coverage 96.7% (gate 95%). Found during the same NLP adversarial sweep as the label-column gap in this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…x/size geometry (#314) * fix(validators): extend 0-record + label-column guards to vision categories Adversarial image-classification ingestion (re-verified against the merged develop chain) showed the #303 zero-record guard and #313 label-column guard were wired NLP-only, so vision still had the same gaps: - header-only / empty CSV -> 0 records -> orphan empty table + late "rows already in the database" failure (no IngestableRecordsValidator). - missing configured label column -> records clean to label=None -> backend rejects each row with HTTP 400 "label: may not be null" (no LabelColumnValidator). Fix: - New `_zero_record_validator` helper wires IngestableRecordsValidator (file_subdir="images") into ALL file-bearing vision categories — image_classification, object_detection, semantic_segmentation, keypoint_detection — so a zero-record vision manifest fails fast at preflight. - LabelColumnValidator added to image_classification ONLY: it is the only vision category whose label is a CSV column. Object detection / segmentation / keypoint source labels from XML / masks / annotation files, so adding it there would wrongly reject every such dataset. - TextContentValidator stays NLP-only (it decodes UTF-8 text — meaningless for images). Tests: mapping assertions (image gets both guards; OD/seg/keypoint get the zero-record guard only, never LabelColumn); behavioral end-to-end proving image empty-CSV -> IngestableRecords reject and missing-label-column -> LabelColumn reject, valid passes; updated the prior NLP-only content-hygiene assertion. Full suite: 1209 passed, 1 xfailed, total coverage 96.7% (gate 95%). Follow-up to #303 / #313, extending the same hardening from NLP to vision. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(validators): reject out-of-bounds bboxes and size/image mismatches (object detection) Adversarial object-detection ingestion (against the latest chain) found two PascalVOCXMLValidator gaps — both let malformed annotations through to training: 1. Bounding box outside the image: coords were checked non-negative and xmin<xmax / ymin<ymax, but NEVER bounded by the declared <size>. A box with xmax/ymax past the image edge (e.g. 9999 on a 64x64 image) passed. 2. Declared <size> never cross-checked against the ACTUAL image: an annotation declaring 128x128 for a real 64x64 image passed, silently corrupting the coordinate scaling training relies on (and letting a bbox "within" a too-large declared size sit outside the real image). Fix: - Thread the declared width/height from <size> into the bbox check; error when xmax > width or ymax > height. - New `_validate_size_matches_image`: locate the image at <SRC>/images/<filename> (annotation's <filename>, else the xml stem + jpg/jpeg/png), read its real dimensions, and error on a mismatch with the declared <size>. Best-effort: a benign skip when the image is absent/unreadable or Pillow is missing (FilePairing/FileType own the missing-image case). Tests: bbox exceeding declared width/height fails; within-size passes; declared size matching the actual image passes; mismatch fails; absent image skips the cross-check. Full suite: 1215 passed, 1 xfailed, coverage 96.6% (gate 95%); xml_validator 99%. Part of #314 — extends the vision input-validation hardening to OD annotations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(validators): locate OD image by XML stem, not <filename> (bugbot #314) The size<->image cross-check resolved the image from the annotation's <filename> element first. But the ingestor pairs images to annotations by STEM (FilePairingValidator: annotations/<stem>.xml <-> images/<stem>.<ext>), so a stale/wrong <filename> could point the check at a different on-disk image that happens to match the declared <size> while the actually-paired image still disagrees. Resolve the image by XML stem only (jpg/jpeg/png), matching the real pairing. Tests updated to pair ann.xml with ann.jpg. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(validators): resolve OD images dir from SRC_PATH, not xml parent.parent (bugbot #314) `_validate_size_matches_image` derived the images folder as `file_path.parent.parent/images`, which only holds when the XML sits exactly at `<root>/annotations/<stem>.xml`. `validate()` discovers XML via a recursive `**/*.xml` glob, so a root-level or deeper-nested XML would resolve the wrong images tree (or skip the check). Use `SRC_PATH/images` — the same root FileTypeValidator(path="images") uses — for a consistent, correct lookup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(validators): validate seg masks + bound keypoint coords (vision sweep) Adversarial testing of the remaining vision categories found three gaps where malformed data reached training: Semantic segmentation — masks were unvalidated (ImageResolutionValidator only scans <SRC>/images): 1. a mask whose resolution differs from its image passed (breaks pixel-wise labels); 2. a corrupt / unreadable mask passed (only the .png extension was checked). Fix: ImageResolutionValidator gains a `subdir` param; the seg factory adds a second instance with subdir="masks" (name "Mask Resolution Validator"), so masks must be readable and share the images' target resolution. Keypoint detection: 3. keypoint coordinates past the image edge passed (the validator never saw the image size; coords were only checked non-negative). Fix: thread the declared target_size into KeypointAnnotationValidator as `expected_resolution`; coords above width/height are now rejected. Not changed: keypoint Visibility is still restricted to 0/1. Whether to accept COCO's 0/1/2 is a product/format decision (could let through data the runtime doesn't support), so it's left pending confirmation rather than silently relaxed. Tests: mask subdir validation (valid / wrong-res / corrupt), keypoint coord bounds (within / out-of-bounds / no-resolution no-op), and factory wiring (seg has images+masks resolution validators; keypoint annotation validator gets target_size). Full suite: 1224 passed, 1 xfailed, coverage 96.6% (gate 95%). Completes the vision input-validation sweep in this PR (image, object detection, semantic segmentation, keypoint). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(validators): keypoint bounds are half-open [0,W)/[0,H) (bugbot #314) The upper-bound coord check used `x > width`, so a coordinate exactly equal to the width/height passed — but pixel indices run 0..W-1, so `x == W` is the first index past the image (asymmetric with the inclusive-0 lower bound). Use `x >= width` / `y >= height` (half-open [0, W) / [0, H)). Boundary test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(validators): report keypoint negative + out-of-bounds independently (bugbot #314) The bounds check was an `elif` after the negative-coordinate check, so a row like (-1, 9999) reported only the negative x and hid the out-of-bounds y until a re-ingest. Run the two checks independently so both surface in one pass. Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Summary
Adversarial end-to-end MLM ingestion (dev cluster,
ingestor:0.3.12) fed the validator chain malformed-but-realistic datasets. The tokenizer checks held up, but several input-hygiene gaps let bad data slip past pre-ingestion validation and fail late with misleading messages. This hardens them at the validator level (reproduced via temp CSVs + temp files driving the validators directly), so each is caught before the table is created — no orphan empty table, no misleading "rows already in the database".Fixes
MUST FIX — "0 ingestable records" (two cases)
Finding: an empty/header-only CSV passes all current validators, creates the table, ingests 0 rows, then fails late at the edge-label step with
"…the dataset was NOT registered (its rows are already in the database)"— but there are 0 rows. Same misleading late error when a CSV references files that don't exist and all are missing (every record skipped at transfer → 0 transferred → orphan empty table). PR #250 only caught a totally empty file; a header-only CSV slipped through.Fix:
IngestableRecordsValidator(wired into the NLP factories via_nlp_content_validators) rejects:"No data rows found in CSV …""No referenced data files could be found; nothing to ingest"(cross-checks the CSV filename column againstSRC_PATH/<subdir>/, early-exiting on the first file that resolves)validate_databeforecreate_table, so a rejected ingest leaves no orphan table (the bug: validator-rejected ingest leaves an orphaned table; no rollback blocks the next ingest #260 guard, at the zero-records gate).base.py:_rows_state_clause(inserted_records)rephrases the three registration-failure messages so they can never claim "its rows are already in the database" when 0 rows were ingested ("no rows were ingested, so nothing was left in the database"). The"NOT registered"wording is kept.Repro (tests):
test_header_only_csv_is_rejected,test_empty_zero_byte_csv_is_rejected,test_all_referenced_files_missing_is_rejected,test_mlm_header_only_csv_fails_before_table_creation+…all_files_missing…(assertcreate_tableis never called),test_zero_inserted_registration_failure_message_is_truthful.SHOULD FIX — NLP text-content validator (binary / empty docs)
Finding: the File Type Validator only checks the extension. A
.txtholding non-UTF-8/binary bytes is ingested silently (5/5 "successfully processed" with garbage); an empty.txtis ingested with no warning. The user only finds out at training time.Fix: new
TextContentValidator, gated ontext_classification/token_classification/masked_language_modeling. Samples the referenced text files (deterministic strided sample + per-file byte cap) and:Repro (tests):
test_binary_content_is_rejected,test_non_utf8_bytes_are_rejected,test_empty_file_is_warned_not_rejected,test_whitespace_only_file_is_warned,test_multibyte_char_split_at_byte_cap_is_not_a_false_positive,test_sampling_bounds_files_checked.NICE TO HAVE — within-CSV duplicate detection
Finding: duplicate rows in the CSV (same filename twice) ingest as separate records; the existing Duplicate Validator only checks against the existing table, not within the incoming CSV.
Fix:
DuplicateValidatornow also warns (no hard-fail — repeats may be intentional) when a filename appears more than once in the incoming CSV. Bounded (reads only the filename column); no-op for non-CSV / tabular inputs without a filename column.Repro (tests):
test_within_csv_duplicate_filenames_warn,…case_insensitive_column,…no_filename_column_is_noop,…none_input_is_noop.Implementation notes
map_validators(per-category factories inmodalities/validators.py), gated to NLP categories only (no impact on image/tabular).text_profile._sampleandfile_transfer._has_extension.BaseValidator._match_columncentralises the case-insensitive header lookup (also fixed an existing unusedtyping.Listimport induplicate_validator.py).Tests
pytest tests/ -q --cov=tracebloc_ingestor --cov-fail-under=95→ 1183 passed, 1 xfailed, total coverage 96.7% (gate 95%).🤖 Generated with Claude Code
Note
Medium Risk
Changes the pre-ingestion validation path and registration error text for NLP categories; behavior is well-covered by new tests but affects a critical ingest gate before table creation.
Overview
Hardens preflight validation for NLP ingests so bad manifests fail before
create_table, with clearer errors instead of late registration failures that wrongly claim rows are already in the database.Zero-record fail-fast: Adds
IngestableRecordsValidatorfor text/token classification and MLM. It rejects header-only or empty CSVs and manifests where every referenced file is missing underSRC_PATH(using the same_safe_joinrules as transfer). Wired via_nlp_content_validatorsinmodalities/validators.py(texts/vssequences/per category).NLP text content: Adds
TextContentValidatoron the same categories—samples staged files, fails on binary/NUL/non-UTF-8 bytes, warns on empty/whitespace-only docs, with bounded sampling and safe path handling.Registration messaging:
base.pyintroduces_rows_state_clause(inserted_records)so edge-label, global-meta, and prepare failures describe DB state accurately (e.g. “no rows were ingested…” when count is 0).Duplicate CSV filenames:
DuplicateValidatorwarns (does not fail) when the samefilenameappears multiple times in the incoming CSV; addsBaseValidator._match_columnfor case-insensitive headers.Reviewed by Cursor Bugbot for commit 041ed13. Bugbot is set up for automated code reviews on this repo. Configure here.