From 551019c4aae64cf1308c915a6997bef859fb58e3 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Thu, 9 Jul 2026 20:12:54 +0200 Subject: [PATCH 1/2] fix(preflight): resolve labels.csv filename by column name, not position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CrossCheckLabels (image_classification preflight) read the image filename from rec[0] — the first column, positionally. But the ingestor reads it BY NAME: record.get("filename") over the header-keyed record (file_transfer.py / record_processor.py), order-independent, and every template's labels.csv names the column "filename". So a `label,filename` header (filename present but not first — a layout the cluster ingests cleanly) made the CLI read the LABEL value ("cat") as the filename, miss every row, and reject with exit 3 ("N labels.csv row(s) reference images that aren't in images/") — a false reject that also misleadingly listed label values as missing images. Violated the "never stricter than the ingestor" preflight contract. Fix: resolve the filename column by name — exact "filename" first, then case-insensitive + whitespace-trimmed (the ingestor's _match_column rule) — and read that column's index. Falls back to column 0 when there's no filename-ish column (a malformed layout the ingestor fails on regardless; not this check's job to diagnose). Short/ragged rows without that cell are skipped. Tests: the existing test's unrealistic `image_id,label` fixture corrected to the real `filename,label`; new TestCrossCheckLabels_FilenameColumnNotFirst pins that `label,Filename` (not first, mixed case) resolves correctly and does NOT read labels as filenames (fails against the old rec[0]); TestFilenameColIndex covers first/not-first/case/whitespace/fallback. Full suite green; gofmt -s / errcheck / ineffassign / misspell clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/push/preflight.go | 38 +++++++++++++++++++---- internal/push/preflight_test.go | 55 ++++++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/internal/push/preflight.go b/internal/push/preflight.go index a2810eaf..5f217294 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -247,8 +247,11 @@ func ValidateImages(images []string, expectedW, expectedH int) error { // direction for image_classification) — the caller may surface them as a // note. // -// filenameColumn is the CSV's first column (the ingestor reads filenames -// positionally from the id column of labels.csv). +// The image filename is read from the column NAMED "filename", not positionally: +// the ingestor does record.get("filename") over the header-keyed record +// (file_transfer.py / record_processor.py), so a `label,filename` header (filename +// not first) resolves to that column — reading rec[0] would treat the LABEL value +// as the filename and false-reject a layout the cluster ingests cleanly. func CrossCheckLabels(csvPath string, images []string, extension string) (missing []string, orphans []string, err error) { f, err := os.Open(csvPath) if err != nil { @@ -268,12 +271,14 @@ func CrossCheckLabels(csvPath string, images []string, extension string) (missin } referenced := make(map[string]bool) - if _, err := r.Read(); err != nil { // header + header, err := r.Read() + if err != nil { if errors.Is(err, io.EOF) { return nil, nil, nil // emptiness is CheckHasDataRows' diagnostic } return nil, nil, fmt.Errorf("reading %s: %w", filepath.Base(csvPath), err) } + fnIdx := filenameColIndex(header) for { rec, err := r.Read() if errors.Is(err, io.EOF) { @@ -282,10 +287,10 @@ func CrossCheckLabels(csvPath string, images []string, extension string) (missin if err != nil { return nil, nil, fmt.Errorf("reading %s: %w", filepath.Base(csvPath), err) } - if len(rec) == 0 { - continue + if fnIdx >= len(rec) { + continue // short/ragged row — no filename cell to check } - name := strings.TrimSpace(rec[0]) + name := strings.TrimSpace(rec[fnIdx]) if name == "" { continue } @@ -311,6 +316,27 @@ func CrossCheckLabels(csvPath string, images []string, extension string) (missin return missing, orphans, nil } +// filenameColIndex returns the header index of the "filename" column — the one the +// ingestor reads each image's filename from (record.get("filename"), order- +// independent). Matches "filename" exactly first, then case-insensitively with +// surrounding whitespace stripped (the ingestor's _match_column rule that +// IngestableRecordsValidator applies to the filename column). Falls back to 0 when +// there's no filename-ish column — a malformed image_classification labels.csv the +// ingestor fails on regardless, which is not this check's job to diagnose. +func filenameColIndex(header []string) int { + for i, h := range header { + if strings.TrimSpace(h) == "filename" { + return i + } + } + for i, h := range header { + if strings.EqualFold(strings.TrimSpace(h), "filename") { + return i + } + } + return 0 +} + // CheckAnnotationPairing previews the ingestor's FilePairingValidator // (file_pairing_validator.py) for object_detection: every image must have // an annotation with the same filename stem and vice versa — a mismatch in diff --git a/internal/push/preflight_test.go b/internal/push/preflight_test.go index ed546f4e..7621b356 100644 --- a/internal/push/preflight_test.go +++ b/internal/push/preflight_test.go @@ -163,8 +163,10 @@ func TestCrossCheckLabels(t *testing.T) { // One row exact, one extensionless (the ingestor appends the dataset // extension — the check must mirror that), one missing. csvPath := filepath.Join(dir, "labels.csv") + // Realistic image_classification labels.csv: the ingestor reads the image name + // from the column NAMED "filename" (record.get("filename")). if err := os.WriteFile(csvPath, - []byte("image_id,label\na.jpg,cat\nb,dog\nghost.jpg,cat\n"), 0o644); err != nil { + []byte("filename,label\na.jpg,cat\nb,dog\nghost.jpg,cat\n"), 0o644); err != nil { t.Fatal(err) } images := []string{filepath.Join(imgs, "a.jpg"), filepath.Join(imgs, "b.jpg"), filepath.Join(imgs, "extra.jpg")} @@ -180,6 +182,57 @@ func TestCrossCheckLabels(t *testing.T) { } } +// The image filename is resolved by the "filename" COLUMN NAME, not positionally — +// so a `label,filename` header (filename not first, a layout the ingestor accepts +// via record.get("filename")) must not false-reject. Reading rec[0] would treat the +// label value ("cat"/"dog") as the filename and flag every row missing (exit 3). +func TestCrossCheckLabels_FilenameColumnNotFirst(t *testing.T) { + dir := t.TempDir() + imgs := filepath.Join(dir, "images") + if err := os.MkdirAll(imgs, 0o755); err != nil { + t.Fatal(err) + } + for _, n := range []string{"a.jpg", "b.jpg", "extra.jpg"} { + if err := os.WriteFile(filepath.Join(imgs, n), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + } + csvPath := filepath.Join(dir, "labels.csv") + // filename is the SECOND column, and mixed-case to exercise the ci match. + if err := os.WriteFile(csvPath, + []byte("label,Filename\ncat,a.jpg\ndog,b\ncat,ghost.jpg\n"), 0o644); err != nil { + t.Fatal(err) + } + images := []string{filepath.Join(imgs, "a.jpg"), filepath.Join(imgs, "b.jpg"), filepath.Join(imgs, "extra.jpg")} + missing, orphans, err := CrossCheckLabels(csvPath, images, ".jpg") + if err != nil { + t.Fatal(err) + } + if len(missing) != 1 || missing[0] != "ghost.jpg" { + t.Errorf("missing = %v, want [ghost.jpg] — the label values must NOT be read as filenames", missing) + } + if len(orphans) != 1 || orphans[0] != "extra.jpg" { + t.Errorf("orphans = %v, want [extra.jpg]", orphans) + } +} + +func TestFilenameColIndex(t *testing.T) { + cases := []struct { + header []string + want int + }{ + {[]string{"filename", "label"}, 0}, // by name, first + {[]string{"label", "filename"}, 1}, // by name, not first (the bug) + {[]string{"label", " Filename "}, 1}, // case + whitespace insensitive + {[]string{"image_id", "label"}, 0}, // no filename-ish column → fallback to 0 + } + for _, c := range cases { + if got := filenameColIndex(c.header); got != c.want { + t.Errorf("filenameColIndex(%v) = %d, want %d", c.header, got, c.want) + } + } +} + func TestCheckAnnotationPairing(t *testing.T) { imgs := []string{"images/a.jpg", "images/b.jpg"} anns := []string{"annotations/a.xml", "annotations/c.xml"} From 21258c9aa0ba567ab83fa09fe260b33db31b350b Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Fri, 10 Jul 2026 12:34:10 +0200 Subject: [PATCH 2/2] fix(preflight): resolve the image column as filename else data_id, like the ingestor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses @saadqbal's #207 review. The ingestor resolves the image file key as `filename` else `data_id` (image_paths / image_loader), position-independent for both — so a valid `label,data_id` CSV (no filename column) was falling back to index 0 (the label column) and false-rejecting (exit 3) a dataset the cluster ingests cleanly. imageFileColIndex (renamed from filenameColIndex) now matches filename-else-data_id with the same exact-then-case-insensitive-trimmed rule; filename wins when both are present; it falls back to 0 only when NEITHER exists (a labels.csv the ingestor rejects at validate_data anyway). Added a label,data_id CrossCheckLabels regression test + data_id cases to the index test. Co-Authored-By: Claude Opus 4.8 --- internal/push/preflight.go | 41 +++++++++++++++----------- internal/push/preflight_test.go | 52 ++++++++++++++++++++++++++++----- 2 files changed, 69 insertions(+), 24 deletions(-) diff --git a/internal/push/preflight.go b/internal/push/preflight.go index 5f217294..5f7fffbf 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -278,7 +278,7 @@ func CrossCheckLabels(csvPath string, images []string, extension string) (missin } return nil, nil, fmt.Errorf("reading %s: %w", filepath.Base(csvPath), err) } - fnIdx := filenameColIndex(header) + fnIdx := imageFileColIndex(header) for { rec, err := r.Read() if errors.Is(err, io.EOF) { @@ -316,22 +316,29 @@ func CrossCheckLabels(csvPath string, images []string, extension string) (missin return missing, orphans, nil } -// filenameColIndex returns the header index of the "filename" column — the one the -// ingestor reads each image's filename from (record.get("filename"), order- -// independent). Matches "filename" exactly first, then case-insensitively with -// surrounding whitespace stripped (the ingestor's _match_column rule that -// IngestableRecordsValidator applies to the filename column). Falls back to 0 when -// there's no filename-ish column — a malformed image_classification labels.csv the -// ingestor fails on regardless, which is not this check's job to diagnose. -func filenameColIndex(header []string) int { - for i, h := range header { - if strings.TrimSpace(h) == "filename" { - return i - } - } - for i, h := range header { - if strings.EqualFold(strings.TrimSpace(h), "filename") { - return i +// imageFileColIndex returns the header index of the column the ingestor reads each +// image's file key from: "filename" if present, else "data_id" — the ingestor's own +// precedence (image_paths.prepare_classification_pytorch_image_df / image_loader), +// position-independent for both. A label,data_id CSV (no filename column) is ingested +// cleanly by the cluster, so matching only "filename" and falling back to index 0 +// would read the label column as filenames and false-reject it (exit 3). +// +// Each name is matched exactly first, then case-insensitively with surrounding +// whitespace stripped (the ingestor's _match_column rule). "filename" wins over +// "data_id" when both resolve. Falls back to 0 only when NEITHER column exists — a +// labels.csv the ingestor rejects at validate_data regardless, not this check's job +// to diagnose. +func imageFileColIndex(header []string) int { + for _, want := range []string{"filename", "data_id"} { + for i, h := range header { + if strings.TrimSpace(h) == want { + return i + } + } + for i, h := range header { + if strings.EqualFold(strings.TrimSpace(h), want) { + return i + } } } return 0 diff --git a/internal/push/preflight_test.go b/internal/push/preflight_test.go index 7621b356..6c3381a8 100644 --- a/internal/push/preflight_test.go +++ b/internal/push/preflight_test.go @@ -216,19 +216,57 @@ func TestCrossCheckLabels_FilenameColumnNotFirst(t *testing.T) { } } -func TestFilenameColIndex(t *testing.T) { +// A `label,data_id` header (no filename column) is ingested cleanly by the cluster — +// the ingestor resolves the image column as filename ELSE data_id. CrossCheckLabels +// must resolve data_id, not fall back to index 0 (the label column), which would read +// "cat"/"dog" as filenames and false-reject every row (exit 3). Regression for Asad's +// #207 review. +func TestCrossCheckLabels_DataIDColumn(t *testing.T) { + dir := t.TempDir() + imgs := filepath.Join(dir, "images") + if err := os.MkdirAll(imgs, 0o755); err != nil { + t.Fatal(err) + } + for _, n := range []string{"a.jpg", "b.jpg", "extra.jpg"} { + if err := os.WriteFile(filepath.Join(imgs, n), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + } + csvPath := filepath.Join(dir, "labels.csv") + // data_id instead of filename, and not first — a layout the ingestor accepts. + if err := os.WriteFile(csvPath, + []byte("label,data_id\ncat,a.jpg\ndog,b\ncat,ghost.jpg\n"), 0o644); err != nil { + t.Fatal(err) + } + images := []string{filepath.Join(imgs, "a.jpg"), filepath.Join(imgs, "b.jpg"), filepath.Join(imgs, "extra.jpg")} + missing, orphans, err := CrossCheckLabels(csvPath, images, ".jpg") + if err != nil { + t.Fatal(err) + } + if len(missing) != 1 || missing[0] != "ghost.jpg" { + t.Errorf("missing = %v, want [ghost.jpg] — data_id must resolve; label values must NOT be read as filenames", missing) + } + if len(orphans) != 1 || orphans[0] != "extra.jpg" { + t.Errorf("orphans = %v, want [extra.jpg]", orphans) + } +} + +func TestImageFileColIndex(t *testing.T) { cases := []struct { header []string want int }{ - {[]string{"filename", "label"}, 0}, // by name, first - {[]string{"label", "filename"}, 1}, // by name, not first (the bug) - {[]string{"label", " Filename "}, 1}, // case + whitespace insensitive - {[]string{"image_id", "label"}, 0}, // no filename-ish column → fallback to 0 + {[]string{"filename", "label"}, 0}, // filename by name, first + {[]string{"label", "filename"}, 1}, // filename by name, not first (the original bug) + {[]string{"label", " Filename "}, 1}, // case + whitespace insensitive + {[]string{"label", "data_id"}, 1}, // no filename → data_id (the ingestor's fallback) + {[]string{"label", "Data_ID"}, 1}, // data_id, case-insensitive + {[]string{"data_id", "filename", "label"}, 1}, // filename wins over data_id when both present + {[]string{"image_id", "label"}, 0}, // neither → fallback to 0 } for _, c := range cases { - if got := filenameColIndex(c.header); got != c.want { - t.Errorf("filenameColIndex(%v) = %d, want %d", c.header, got, c.want) + if got := imageFileColIndex(c.header); got != c.want { + t.Errorf("imageFileColIndex(%v) = %d, want %d", c.header, got, c.want) } } }