Uh oh!
There was an error while loading. Please reload this page.
fix(push): reject image datasets with no "filename" column in dry-run (#371) - #373
Merged
Merged
Conversation
…#371) Image `data ingest` preflight green-lit a labels.csv whose file column wasn't named `filename` (e.g. `image_id`): imageFileColIndex fell back to column 0, read the label values as filenames, found the files, and passed. The cluster then reads each image via a case-sensitive record.get("filename") at transfer time (data-ingestors file_transfer.py:179), finds no key, and fails every row post-upload with "No filename found in record" (exit 9). The ingestor's own preflight (validate_data) doesn't catch this — it only surfaces at transfer — so this was a genuine accept-locally / fail-after-upload parity gap, invisible to the golden harness (goldens are generated from validate_data, which accepts image_id,label). - Enforce the contract's requires_filename_column up front for the image family (CheckImageFilenameColumn), mirroring the text family's check. - Drop imageFileColIndex's data_id false-accept and column-0 fallback — the ingestor never aliases another column to filename, so a data_id-only manifest fails at transfer identically to image_id. - Correct all 17 imgc-* parity fixtures to `filename`, add imgc-no-filename-col to pin the divergence (ingestor preflight accepts, CLI rejects the doomed upload), regenerate goldens against the pin, fix unit-test CSVs + help text. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
saadqbal
left a comment
Collaborator
There was a problem hiding this comment.
Good, careful fix and the parity-pin approach is nice. A few things worth checking before merge — flagged inline. Main one: this reverses the data_id handling from #207, so I'd want the ingestor source confirming data_id-only really fails transfer, otherwise it's a new false-reject. Also the case-insensitive match leaves the Filename variant still failing after upload (the same bug class), and we decode every image before this cheap header check runs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…#373) Asad's review of #373: - data_id (comment): confirmed against the ingestor source that a data_id-only image manifest fails at TRANSFER, so this is not a false-reject. The ingest copy reads record.get("filename") case-sensitively (data-ingestors record_processor.py:279 -> file_transfer.py:179) with no data_id fallback; the filename-vs-data_id resolver #207 cited (prepare_classification_pytorch_image_df) lives in tracebloc-client and runs at TRAINING time over the already-ingested table, not the ingest copy. Documented this in the imageFileColIndex doc comment. - Filename case variant: CheckImageFilenameColumn / imageFileColIndex now match EXACTLY (trimmed, case-sensitive), mirroring the transfer read, so a Filename/FILENAME header is rejected up front with a "lowercase it" message instead of being green-lit into the same doomed upload. Adds the imgc-uppercase-filename-col parity case (ingestor preflight accepts, CLI rejects) + unit coverage. - Decode cost: moved the cheap manifest/header checks (incl. CheckImageFilenameColumn) AHEAD of the per-image ValidateImages decode, so a structurally doomed manifest fails before we open a single image. Removes the now-contradicted TestCrossCheckLabels_FilenameColumnNotFirst (mixed-case), superseded by the case-sensitive FilenameNotFirst. Goldens regenerated against the pin (sync-validator-goldens.sh --check in sync). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6f0f96b pushed internal/push/preflight.go to 1714 lines, over the 1700-line file-budget ratchet (the failing Lint job). Consolidated the overlapping filename-column doc comments — the full rationale (case-sensitive transfer read, data_id-is-training-time, source citations) now lives once in imageFileColIndex, with CheckImageFilenameColumn referencing it — and merged the reorder note. 1698 lines now; comments only, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
saadqbal
approved these changes
Jul 21, 2026
saadqbal
commented
Jul 21, 2026
Collaborator
/fr-pass |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes#371.
tb data ingestfor image tasks green-lit alabels.csvwhose file column wasn't namedfilename(e.g.image_id): the dry-run printed✔ Dry-run complete, then the real ingest failed cluster-side with exit 9 (file_transfer.py:182 | No filename found in record, 0 records ingested). An accept-locally / fail-after-upload parity gap (epic tracebloc/backend#1142 Theme D; sibling of #352).Root cause: image preflight resolved the file column via
imageFileColIndex, which fell back to column 0 when neitherfilenamenordata_idwas present — reading the label values as filenames and passing. The cluster reads each image via a case-sensitiverecord.get("filename")at transfer time and accepts exactly one key,filename(it never aliases another column), soimage_id/data_idresolve to nothing. The ingestor's own preflight (validate_data) doesn't catch this, which also blinded the golden harness (goldens come fromvalidate_data, which acceptsimage_id,label).What changed
requires_filename_columnup front for the image family (CheckImageFilenameColumn) — reject in the dry-run with an actionable message, mirroring the text family's existing check.data_idfalse-accept + column-0 fallback inimageFileColIndex(require literallyfilename, case-insensitive + trimmed, like the ingestor's_match_column).data_idwas never a real alias — it fails at transfer identically toimage_id.imgc-*fixtures tofilename; addedimgc-no-filename-colto pin the divergence (ingestor preflight accepts, CLI rejects the doomed upload); regeneratedgoldens.jsonagainst the pin.walk.gohelp text.Type
Bug fix.
Test plan
go test ./...— green.go test ./internal/push/ -run TestValidatorParity— green, incl. the newimgc-no-filename-colcase.PYTHON=<di-venv> DATA_INGESTORS_DIR=<pinned> scripts/sync-validator-goldens.sh --check→ "validator goldens in sync". The regeneration diff was surgical: only the new case added +imgc-label-missing's golden error text corrected (image_id, target→filename, target); no other verdict/value changed.image_id,labeldataset → dry-run now rejects at "Step 1/3 Check your data" (exit 3), before touching the workspace: "labels.csv has no "filename" column (columns: image_id, label) — image tasks match each row to its file by that column … Rename your file column to "filename" and re-run."filename,labeldataset → dry-run passes (exit 0), "✔ Dry-run complete".Checklist
CheckImageFilenameColumn,imageFileColIndex,CrossCheckLabels, new parity case)--checkin sync)gofmt/go vetcleanRelated
filenamecolumn is matched case-insensitively at preflight but read case-sensitively at transfer (chore: back-merge main → develop (adopt cobra 1.10.2 + reconcile drift) #340-class asymmetry), soFilenamepasses preflight yet fails transfer. Best fixed ingestor-side; out of scope here.🤖 Generated with Claude Code
Note
Medium Risk
Changes image preflight validation and dry-run behavior for all image tasks; stricter than before but prevents known post-upload failures. No auth or cluster execution path changes beyond earlier local rejection.
Overview
Fixes #371: image ingest dry-runs could pass when
labels.csvusedimage_idor other file columns, then fail cluster-side at transfer with exit 9.CheckImageFilenameColumnenforces the layout contract (requires_filename_column): manifests must have an exact lowercasefilenamecolumn (whitespace-trimmed). Wrong names (image_id,data_id) and case variants (Filename) are rejected with actionable errors before upload.imageFileColIndexno longer falls back to column 0 or treatsdata_idas an alias—onlyfilenamematches, aligned with case-sensitiverecord.get("filename")at transfer.PreflightDatasetruns manifest/header checks (including the new one) before per-image decode for fail-fast behavior.Tests and parity fixtures use
filenameheaders; new parity casesimgc-no-filename-colandimgc-uppercase-filename-coldocument deliberate CLI-reject / ingestor-accept gaps.Reviewed by Cursor Bugbot for commit 8ea2cdb. Bugbot is set up for automated code reviews on this repo. Configure here.