diff --git a/internal/push/parity_golden_test.go b/internal/push/parity_golden_test.go index 2006b39d..186db758 100644 --- a/internal/push/parity_golden_test.go +++ b/internal/push/parity_golden_test.go @@ -4,11 +4,13 @@ import ( "encoding/json" "os" "path/filepath" + "slices" "sort" "testing" ) -// The validator-parity harness (backend#828 P3). Two assertions per case: +// The validator-parity harness (backend#828 P3; value-level from backend#1009). +// Per case: // // 1. the Go preflight's verdict matches the manifest's cli_verdict — // pins the CLI side; @@ -17,6 +19,11 @@ import ( // manifest's ingestor_verdict — so when the ingestor's rules change, // regenerating the goldens fails this test until the manifest (and, // where needed, the Go preview) is consciously updated. +// 3. for cases flagged value_parity, the Go preview's VALUE-level read of +// the label column (resolved header + row count + class set) equals the +// REAL ingestor's — the only assertion that catches accept/accept with +// divergent stored data (data-ingestors #340: a case-/whitespace- +// mismatched label passes both verdicts, then reads null in-cluster). // // Deliberate divergences (the CLI previewing read-/transfer-time failures // the ingestor's preflight can't see) are explicit in the manifest, never @@ -32,6 +39,7 @@ type parityCase struct { Schema map[string]string `json:"schema"` CLIVerdict string `json:"cli_verdict"` IngestorVerdict string `json:"ingestor_verdict"` + ValueParity bool `json:"value_parity"` Note string `json:"note"` } @@ -45,6 +53,11 @@ func TestValidatorParity(t *testing.T) { Verdicts map[string]struct { Verdict string `json:"verdict"` Errors []string `json:"errors"` + Values *struct { + Resolved string `json:"resolved_label"` + RowCount int `json:"row_count"` + Classes []string `json:"classes"` + } `json:"values"` } `json:"verdicts"` } mustLoad(t, filepath.Join("testdata", "parity", "goldens.json"), &goldens) @@ -64,10 +77,56 @@ func TestValidatorParity(t *testing.T) { if got != c.CLIVerdict { t.Errorf("Go preflight = %q, manifest expects %q (note: %s)", got, c.CLIVerdict, c.Note) } + + if !c.ValueParity { + return + } + // Value-level parity (backend#1009): the Go preview must read the + // SAME label header, row count, and class set the real ingestor + // does. Catches accept/accept-with-divergent-label (#340). + if golden.Values == nil { + t.Fatalf("case %s is value_parity but goldens.json has no values — "+ + "regenerate with scripts/gen-validator-goldens.py against a data-ingestors "+ + "checkout that includes the #340 label-resolution fix", c.Name) + } + gv := goLabelValues(t, c) + if gv.Resolved != golden.Values.Resolved { + t.Errorf("resolved label: Go preview = %q, ingestor golden = %q "+ + "(the read paths resolve the label column differently — #340 class)", + gv.Resolved, golden.Values.Resolved) + } + if gv.RowCount != golden.Values.RowCount { + t.Errorf("row count: Go preview = %d, ingestor golden = %d", gv.RowCount, golden.Values.RowCount) + } + if !slices.Equal(gv.Classes, golden.Values.Classes) { + t.Errorf("class set: Go preview = %v, ingestor golden = %v", gv.Classes, golden.Values.Classes) + } }) } } +// goLabelValues runs the Go preview's value-level label read for a case, +// deriving the NA-drop / numeric-collapse flags from the label's schema type +// exactly as PreflightDataset does — so the value comparison uses the same +// read semantics the production preflight would. +func goLabelValues(t *testing.T, c parityCase) LabelReadValues { + t.Helper() + csvPath := filepath.Join("testdata", "parity", "cases", c.Name, c.CSV) + schema := c.Schema + if IsTabular(c.Category) && len(schema) == 0 { + if sch, _, _, err := InferSchema(csvPath); err == nil { + schema = sch + } + } + dropNA, collapse := false, false + if IsTabular(c.Category) { + sqlType, inSchema := labelSchemaType(schema, c.LabelColumn) + dropNA = inSchema + collapse = !(inSchema && isStringSQLType(sqlType)) + } + return ReadLabelValues(csvPath, c.LabelColumn, dropNA, collapse) +} + // runGoPreflight runs THE production dispatch (push.PreflightDataset) over // the case — the same code path runDataIngest executes, so a check deleted // or rewired in production fails parity here. diff --git a/internal/push/preflight.go b/internal/push/preflight.go index 0e67e7e5..a2810eaf 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -383,9 +383,54 @@ func TruncateList(items []string, max int) string { // even an empty string is a real class and every distinct trimmed string // counts. The caller derives the two flags from the label's schema type. func CheckLabelDiversity(csvPath, labelColumn string, dropNASentinels, collapseNumeric bool) error { + v := readLabelColumnValues(csvPath, labelColumn, dropNASentinels, collapseNumeric) + // Benign-skip when the column is absent (that's CheckLabelColumn's + // diagnostic) or an unreadable file (another check's) — both leave Found + // false. Two or more classes is diverse enough. + if !v.Found || len(v.Classes) >= 2 { + return nil + } + return fmt.Errorf( + "the label column %q has %d distinct value(s) — a classification dataset needs at "+ + "least 2 classes. The cluster rejects this after the upload; check the labels and re-run.", + labelColumn, len(v.Classes)) +} + +// LabelReadValues is the value-level view of a label column: the header the +// read path RESOLVES the configured name to (case/whitespace-insensitively — +// the ingestor's rule), the sorted distinct classes the ingestor counts, and +// the data-row count. It is what the value-level parity harness pins, so a +// preview that says "N rows, K classes" cannot silently diverge from what the +// ingestor actually reads — the accept/accept-with-divergent-label class the +// verdict-only harness is blind to (data-ingestors #340). +type LabelReadValues struct { + Resolved string `json:"resolved_label"` + Classes []string `json:"classes"` + RowCount int `json:"row_count"` + Found bool `json:"-"` +} + +// ReadLabelValues is the exported value-level read used by the parity harness +// (and, later, the RFC-0002 "check your data" preview). It shares the exact +// read/resolve/NA/collapse rules with CheckLabelDiversity via +// readLabelColumnValues, so the value preview and the diversity verdict cannot +// drift from each other. +func ReadLabelValues(csvPath, labelColumn string, dropNASentinels, collapseNumeric bool) LabelReadValues { + return readLabelColumnValues(csvPath, labelColumn, dropNASentinels, collapseNumeric) +} + +// readLabelColumnValues reads csvPath's label column once and returns its +// value-level view. The column is resolved exactly, then case/whitespace- +// insensitively (mirroring the ingestor's resolve_column rule); each row value +// is whitespace-trimmed; NA sentinels are dropped and numeric values collapsed +// per the caller's flags (see CheckLabelDiversity's doc for how those mirror +// the ingestor's per-column read). Unlike the previous early-exit diversity +// scan, this reads the whole column to build the full class set + row count — +// one scan now backs both the diversity verdict and the value-level preview. +func readLabelColumnValues(csvPath, labelColumn string, dropNASentinels, collapseNumeric bool) LabelReadValues { f, err := os.Open(csvPath) if err != nil { - return nil // unreadable file is another check's diagnostic + return LabelReadValues{} // Found=false: unreadable file is another check's diagnostic } defer func() { _ = f.Close() }() br := bufio.NewReader(f) @@ -396,12 +441,12 @@ func CheckLabelDiversity(csvPath, labelColumn string, dropNASentinels, collapseN r.FieldsPerRecord = -1 header, err := r.Read() if err != nil { - return nil + return LabelReadValues{} } - col := -1 + col, resolved := -1, "" for i, c := range header { if strings.TrimSpace(c) == labelColumn { - col = i + col, resolved = i, strings.TrimSpace(c) break } } @@ -409,21 +454,26 @@ func CheckLabelDiversity(csvPath, labelColumn string, dropNASentinels, collapseN want := strings.ToLower(strings.TrimSpace(labelColumn)) for i, c := range header { if strings.ToLower(strings.TrimSpace(c)) == want { - col = i + col, resolved = i, strings.TrimSpace(c) break } } } if col == -1 { - return nil // benign-skip, like the ingestor + return LabelReadValues{} // Found=false — benign skip, like the ingestor } distinct := map[string]bool{} + rowCount := 0 for { rec, err := r.Read() if errors.Is(err, io.EOF) { break } - if err != nil || len(rec) <= col { + if err != nil { + continue + } + rowCount++ + if len(rec) <= col { continue } v := strings.TrimSpace(rec[col]) @@ -435,22 +485,18 @@ func CheckLabelDiversity(csvPath, labelColumn string, dropNASentinels, collapseN if collapseNumeric { // Numeric inference collapses "1" and "1.0" into one value // in-cluster; normalize the same way before counting. - if f, err := strconv.ParseFloat(v, 64); err == nil { - v = strconv.FormatFloat(f, 'g', -1, 64) + if fv, err := strconv.ParseFloat(v, 64); err == nil { + v = strconv.FormatFloat(fv, 'g', -1, 64) } } distinct[v] = true - if len(distinct) >= 2 { - return nil - } } - if len(distinct) >= 2 { - return nil + classes := make([]string, 0, len(distinct)) + for k := range distinct { + classes = append(classes, k) } - return fmt.Errorf( - "the label column %q has %d distinct value(s) — a classification dataset needs at "+ - "least 2 classes. The cluster rejects this after the upload; check the labels and re-run.", - labelColumn, len(distinct)) + sort.Strings(classes) + return LabelReadValues{Resolved: resolved, Classes: classes, RowCount: rowCount, Found: true} } // knownMediaExtensions mirrors the ingestor's FileExtension.get_all_extensions diff --git a/internal/push/testdata/parity/cases.json b/internal/push/testdata/parity/cases.json index 27ff423a..b54b8729 100644 --- a/internal/push/testdata/parity/cases.json +++ b/internal/push/testdata/parity/cases.json @@ -7,7 +7,8 @@ "csv": "data.csv", "label_column": "label", "cli_verdict": "accept", - "ingestor_verdict": "accept" + "ingestor_verdict": "accept", + "value_parity": true }, { "name": "tabular-dup-header", @@ -56,7 +57,8 @@ 8 ], "cli_verdict": "accept", - "ingestor_verdict": "accept" + "ingestor_verdict": "accept", + "value_parity": true }, { "name": "imgc-bom-labels", @@ -70,7 +72,8 @@ ], "cli_verdict": "accept", "ingestor_verdict": "accept", - "note": "pandas strips the BOM in-cluster \u2014 the CLI must NOT reject what the cluster accepts (cli#71 parity)" + "note": "pandas strips the BOM in-cluster \u2014 the CLI must NOT reject what the cluster accepts (cli#71 parity)", + "value_parity": true }, { "name": "imgc-label-missing", @@ -98,7 +101,8 @@ ], "cli_verdict": "accept", "ingestor_verdict": "accept", - "note": "the ingestor's _match_column is case-insensitive + trimmed \u2014 the CLI must be exactly as loose" + "note": "the ingestor's _match_column is case-insensitive + trimmed \u2014 the CLI must be exactly as loose", + "value_parity": true }, { "name": "imgc-zero-byte", @@ -196,7 +200,8 @@ ], "cli_verdict": "accept", "ingestor_verdict": "accept", - "note": "non-square [W,H]=[8,4]: pins the target_size ORIENTATION end-to-end \u2014 an [H,W] swap on emit (the pre-P3 bug) flips this to reject in-cluster" + "note": "non-square [W,H]=[8,4]: pins the target_size ORIENTATION end-to-end \u2014 an [H,W] swap on emit (the pre-P3 bug) flips this to reject in-cluster", + "value_parity": true }, { "name": "imgc-nonsquare-swapped", @@ -238,7 +243,8 @@ ], "cli_verdict": "accept", "ingestor_verdict": "accept", - "note": "empty-string label IS a class for image categories (keep_default_na=False): the CLI must not be stricter" + "note": "empty-string label IS a class for image categories (keep_default_na=False): the CLI must not be stricter", + "value_parity": true }, { "name": "imgc-dotted-stem", @@ -252,7 +258,8 @@ ], "cli_verdict": "accept", "ingestor_verdict": "accept", - "note": "row 'photo.2024' resolves to photo.2024.jpg via _has_extension (dotted stems are not extensions) \u2014 the CLI's cross-check must mirror, not reject" + "note": "row 'photo.2024' resolves to photo.2024.jpg via _has_extension (dotted stems are not extensions) \u2014 the CLI's cross-check must mirror, not reject", + "value_parity": true }, { "name": "tabular-na-labels", @@ -280,7 +287,8 @@ "extension": ".txt", "cli_verdict": "accept", "ingestor_verdict": "accept", - "note": "pins the text-family dispatch" + "note": "pins the text-family dispatch", + "value_parity": true }, { "name": "tabular-varchar-numeric-labels", @@ -293,7 +301,8 @@ }, "cli_verdict": "accept", "ingestor_verdict": "accept", - "note": "labels '1' vs '1.0' under a VARCHAR label: the ingestor pins dtype=str (no numeric collapse) so they are 2 classes \u2014 pins #152's schema-type-aware collapse (the earlier blanket collapse falsely rejected this)" + "note": "labels '1' vs '1.0' under a VARCHAR label: the ingestor pins dtype=str (no numeric collapse) so they are 2 classes \u2014 pins #152's schema-type-aware collapse (the earlier blanket collapse falsely rejected this)", + "value_parity": true }, { "name": "tabular-float-numeric-labels", diff --git a/internal/push/testdata/parity/goldens.json b/internal/push/testdata/parity/goldens.json index ac7553dd..c67d17e7 100644 --- a/internal/push/testdata/parity/goldens.json +++ b/internal/push/testdata/parity/goldens.json @@ -3,6 +3,14 @@ "verdicts": { "imgc-bom-labels": { "errors": [], + "values": { + "classes": [ + "cat", + "dog" + ], + "resolved_label": "label", + "row_count": 2 + }, "verdict": "accept" }, "imgc-corrupt": { @@ -13,6 +21,14 @@ }, "imgc-dotted-stem": { "errors": [], + "values": { + "classes": [ + "cat", + "dog" + ], + "resolved_label": "label", + "row_count": 2 + }, "verdict": "accept" }, "imgc-dup-header": { @@ -21,6 +37,14 @@ }, "imgc-empty-label": { "errors": [], + "values": { + "classes": [ + "", + "A" + ], + "resolved_label": "label", + "row_count": 2 + }, "verdict": "accept" }, "imgc-header-only": { @@ -31,6 +55,14 @@ }, "imgc-label-case": { "errors": [], + "values": { + "classes": [ + "cat", + "dog" + ], + "resolved_label": "Label", + "row_count": 2 + }, "verdict": "accept" }, "imgc-label-missing": { @@ -51,6 +83,14 @@ }, "imgc-nonsquare": { "errors": [], + "values": { + "classes": [ + "cat", + "dog" + ], + "resolved_label": "label", + "row_count": 2 + }, "verdict": "accept" }, "imgc-nonsquare-swapped": { @@ -61,6 +101,14 @@ }, "imgc-ok": { "errors": [], + "values": { + "classes": [ + "cat", + "dog" + ], + "resolved_label": "label", + "row_count": 2 + }, "verdict": "accept" }, "imgc-res-mismatch": { @@ -123,14 +171,38 @@ }, "tabular-ok": { "errors": [], + "values": { + "classes": [ + "0", + "1" + ], + "resolved_label": "label", + "row_count": 2 + }, "verdict": "accept" }, "tabular-varchar-numeric-labels": { "errors": [], + "values": { + "classes": [ + "1", + "1.0" + ], + "resolved_label": "label", + "row_count": 2 + }, "verdict": "accept" }, "text-clf-ok": { "errors": [], + "values": { + "classes": [ + "neg", + "pos" + ], + "resolved_label": "label", + "row_count": 2 + }, "verdict": "accept" } } diff --git a/scripts/gen-validator-goldens.py b/scripts/gen-validator-goldens.py index f87aebb5..99ed3b20 100644 --- a/scripts/gen-validator-goldens.py +++ b/scripts/gen-validator-goldens.py @@ -22,6 +22,16 @@ DuplicateValidator are skipped (they check cluster-side state — the table name is validated separately by both sides, and destination-duplicate handling is the cli#70 guard's territory, not a data-hygiene rule). + +For cases the manifest flags ``value_parity`` (and the ingestor accepts), +this also records a VALUE-level golden — the label column the ingestor read +path RESOLVES to, the row count, and the class set it stores — by driving the +REAL read path (CSVIngestor.read_data + the #340 label resolution + +RecordProcessor). parity_golden_test.go then pins that the Go preview reads +exactly the same values, catching accept/accept-with-divergent-label — the +#340 class a verdict alone is blind to (backend#1009). This requires the #340 +fix in the target ingestor; the generator fails loudly without it rather than +pin the bug. """ import json @@ -60,6 +70,59 @@ def infer_schema(csv_path): return {str(c).strip(): "VARCHAR(255)" for c in cols} +def read_label_values(case, csv_path, cfg, options): + """Drive the REAL ingestor read path — CSVIngestor.read_data + the #340 + label-column resolution + RecordProcessor — to capture the value-level view + the parity harness pins: the resolved label header, the row count, and the + sorted distinct classes the ingestor actually stores. This is the only + thing that catches accept/accept-with-divergent-label (the #340 class): + verdicts stay 'accept' while the stored labels silently go null. + + Requires the #340 fix (BaseIngestor._resolve_label_column) in the target + ingestor — without it a case-/whitespace-mismatched label would read null + and this generator would pin the BUG. Fails loudly if it's absent. + """ + from unittest.mock import MagicMock + + from tracebloc_ingestor.ingestors.csv_ingestor import CSVIngestor + + db = MagicMock() + db.config = cfg + file_opts = {k: v for k, v in options.items() if k != "schema"} + ing = CSVIngestor( + database=db, + api_client=MagicMock(), + table_name="parity_t", + schema=options.get("schema", {}) or {}, + label_column=case.get("label_column", "label"), + intent="train", + category=case["category"], + file_options=file_opts, + ) + if not hasattr(ing, "_resolve_label_column"): + sys.exit( + "the target ingestor predates the #340 label-resolution fix; " + "value-level parity requires it. Point DATA_INGESTORS_DIR at a " + "checkout that includes BaseIngestor._resolve_label_column." + ) + records = list(ing.read_data(csv_path)) + # Pin the label column on the first record that CONTAINS it (mirrors the + # ingest loop; sparse-record-safe), then read every row's stored label. + for rec in records: + if ing._resolve_label_column(rec.keys()): + break + labels = [] + for rec in records: + cleaned = ing.process_record(rec) + labels.append(cleaned.get("label") if cleaned else None) + classes = sorted({str(v) for v in labels if v is not None}) + return { + "resolved_label": ing.label_column, + "row_count": len(records), + "classes": classes, + } + + def run_case(case): case_dir = os.path.join(PARITY, "cases", case["name"]) csv_path = os.path.join(case_dir, case["csv"]) @@ -103,10 +166,18 @@ def run_case(case): except Exception as exc: # a raising validator is a rejection too errors.append(f"{type(v).__name__}: raised {exc}") - return { + result = { "verdict": "reject" if errors else "accept", "errors": errors[:6], } + # Value-level golden (data-ingestors #340 class): for cases the manifest + # flags value_parity AND the ingestor accepts, pin the resolved label, + # row count, and class set the REAL read path produces — parity_golden_test + # asserts the Go preview reads exactly these. Only meaningful when accepted + # (a rejected run never reaches the read path). + if case.get("value_parity") and not errors: + result["values"] = read_label_values(case, csv_path, cfg, options) + return result def main(): diff --git a/scripts/sync-validator-goldens.sh b/scripts/sync-validator-goldens.sh index 8cfc02af..9170e1b0 100755 --- a/scripts/sync-validator-goldens.sh +++ b/scripts/sync-validator-goldens.sh @@ -17,17 +17,21 @@ if [[ "${1:-}" == "--check" ]]; then cp "$GOLDENS" "$tmp/committed.json" "$PYTHON" scripts/gen-validator-goldens.py >/dev/null # Compare VERDICTS only — error text may drift harmlessly (and embeds - # fixture paths); verdicts may not. + # fixture paths); verdicts may not. VALUE-level goldens (resolved label + + # row count + class set) carry no paths, so compare them too — a value-only + # drift (the data-ingestors #340 class: verdict unchanged, stored labels + # change) must fail the check, not slip through. if ! "$PYTHON" -c " import json,sys a=json.load(open('$tmp/committed.json'))['verdicts'] b=json.load(open('$GOLDENS'))['verdicts'] -va={k:v['verdict'] for k,v in a.items()}; vb={k:v['verdict'] for k,v in b.items()} -sys.exit(0 if va==vb else 1) +def view(d): return {k:(v['verdict'], v.get('values')) for k,v in d.items()} +sys.exit(0 if view(a)==view(b) else 1) "; then cp "$tmp/committed.json" "$GOLDENS" # restore — check must not mutate - echo "DRIFT: the ingestor's validator verdicts changed. Re-run the generator," >&2 - echo "commit the new goldens, and update cases.json (+ the Go preview) consciously." >&2 + echo "DRIFT: the ingestor's validator verdicts or read-path VALUES changed. Re-run" >&2 + echo "the generator, commit the new goldens, and update cases.json (+ the Go preview)" >&2 + echo "consciously." >&2 exit 1 fi cp "$tmp/committed.json" "$GOLDENS" # keep the committed copy (paths etc. unchanged)