From 2668ad19c54dc7e12a795c59209433f3ccc249c3 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Thu, 9 Jul 2026 17:57:59 +0200 Subject: [PATCH 1/4] feat(data ingest): preview the min-image-size floor + reframe --target-size as a check (#183) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC-0002 §12.9. Mirror data-ingestors #348 in the CLI: image size is a CHECK, never a resize, and there is now a minimum-image-size floor the CLI previews locally so a below-floor dataset fails before the ingest, not after the upload in-cluster. - Schema re-sync: bump scripts/.data-ingestors-ref to the develop SHA that includes #348 and re-vendor internal/schema/ingest.v1.json, which now documents spec.file_options.min_size. Schema-drift check passes on the new ref. - Local floor preview: ValidateImages gains minW/minH and rejects any image with either side below the floor, naming the file, its dimensions, and the floor. PreflightDataset resolves the effective floor (spec.MinSize override, else push.MinImageSize = 32x32 — the exact mirror of ImageResolutionValidator.MIN_IMAGE_SIZE) exactly as the ingestor's __init__ defaults it. The floor takes precedence over a target_size mismatch, matching #348. - --min-size WxH flag plumbs to spec.file_options.min_size (mirrors how --target-size plumbs); the preview uses the effective value. - --target-size copy reframed (flag + interactive prompt): it is the size images must ALREADY be — tracebloc never resizes — and names the min-size floor. - Parity harness: fixtures resized above the 32px floor so each case still tests its intended rule (not the new floor), a dedicated imgc-too-small case added, and goldens regenerated against the pinned ref with the REAL validators. The re-sync also pulled in #338 (the in-cluster BOM-probe fix, which merges below #348), so tabular-bom is now a documented divergence: the pinned ingestor accepts, but the CLI keeps previewing the rejection because the deployed ingestor (v0.5.7) predates #338. Co-Authored-By: Claude Opus 4.8 --- internal/cli/data.go | 23 ++++- internal/cli/interactive.go | 6 +- internal/push/detect.go | 23 ++++- internal/push/preflight.go | 50 +++++++++-- internal/push/preflight_test.go | 59 +++++++++++-- internal/push/spec.go | 33 +++++++ internal/push/spec_test.go | 59 +++++++++++++ internal/push/testdata/parity/cases.json | 82 ++++++++++-------- .../parity/cases/imgc-bom-labels/images/a.jpg | Bin 633 -> 644 bytes .../parity/cases/imgc-bom-labels/images/b.jpg | Bin 633 -> 644 bytes .../parity/cases/imgc-corrupt/images/a.jpg | Bin 633 -> 644 bytes .../cases/imgc-dotted-stem/images/b.jpg | Bin 632 -> 644 bytes .../imgc-dotted-stem/images/photo.2024.jpg | Bin 632 -> 644 bytes .../parity/cases/imgc-dup-header/images/a.jpg | Bin 632 -> 644 bytes .../parity/cases/imgc-dup-header/images/b.jpg | Bin 632 -> 644 bytes .../cases/imgc-empty-label/images/a.jpg | Bin 632 -> 644 bytes .../cases/imgc-empty-label/images/b.jpg | Bin 632 -> 644 bytes .../cases/imgc-header-only/images/a.jpg | Bin 633 -> 644 bytes .../parity/cases/imgc-label-case/images/a.jpg | Bin 633 -> 644 bytes .../parity/cases/imgc-label-case/images/b.jpg | Bin 633 -> 644 bytes .../cases/imgc-label-missing/images/a.jpg | Bin 633 -> 644 bytes .../cases/imgc-label-uniform/images/a.jpg | Bin 633 -> 644 bytes .../cases/imgc-label-uniform/images/b.jpg | Bin 633 -> 644 bytes .../cases/imgc-missing-file/images/a.jpg | Bin 633 -> 644 bytes .../cases/imgc-nonsquare-swapped/images/a.jpg | Bin 632 -> 660 bytes .../cases/imgc-nonsquare-swapped/images/b.jpg | Bin 632 -> 660 bytes .../parity/cases/imgc-nonsquare/images/a.jpg | Bin 632 -> 660 bytes .../parity/cases/imgc-nonsquare/images/b.jpg | Bin 632 -> 660 bytes .../parity/cases/imgc-ok/images/a.jpg | Bin 633 -> 644 bytes .../parity/cases/imgc-ok/images/b.jpg | Bin 633 -> 644 bytes .../cases/imgc-res-mismatch/images/a.jpg | Bin 633 -> 644 bytes .../cases/imgc-res-mismatch/images/odd.jpg | Bin 633 -> 664 bytes .../parity/cases/imgc-too-small/images/a.jpg | Bin 0 -> 632 bytes .../parity/cases/imgc-too-small/images/b.jpg | Bin 0 -> 632 bytes .../parity/cases/imgc-too-small/labels.csv | 3 + .../parity/cases/imgc-zero-byte/images/a.jpg | Bin 633 -> 644 bytes internal/push/testdata/parity/goldens.json | 20 +++-- internal/schema/ingest.v1.json | 7 ++ scripts/.data-ingestors-ref | 2 +- 39 files changed, 302 insertions(+), 65 deletions(-) create mode 100644 internal/push/testdata/parity/cases/imgc-too-small/images/a.jpg create mode 100644 internal/push/testdata/parity/cases/imgc-too-small/images/b.jpg create mode 100644 internal/push/testdata/parity/cases/imgc-too-small/labels.csv diff --git a/internal/cli/data.go b/internal/cli/data.go index 89bdf348..e4e098f2 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -103,6 +103,7 @@ func newDataIngestCmd() *cobra.Command { intent string labelColumn string targetSize string + minSize string schemaFlag string labelPolicy string timeColumn string @@ -231,6 +232,7 @@ Exit codes: NumberOfKeypoints: numberOfKeypoints, }, TargetSizeFlag: targetSize, + MinSizeFlag: minSize, SchemaFlag: schemaFlag, DryRun: dryRun, Overwrite: overwrite, @@ -276,8 +278,12 @@ Exit codes: cmd.Flags().StringVar(&labelColumn, "label-column", "", "name of the label/target column (in labels.csv for image tasks, in the data CSV for tabular)") cmd.Flags().StringVar(&targetSize, "target-size", "", - "image tasks only: resolution as WxH (e.g. 512x512). Default: auto-detected from the first image. "+ - "All images must share this resolution — the ingestor validates it, it does not resize.") + "image tasks only: the resolution your images already are, as WxH (e.g. 512x512). tracebloc never "+ + "resizes — it checks every image is exactly this size and rejects any that differ. Default: "+ + "read from your first image. Images below the --min-size floor are rejected regardless.") + cmd.Flags().StringVar(&minSize, "min-size", "", + "image tasks only: reject images smaller than WxH (e.g. 64x64). Default: 32x32, the smallest "+ + "tracebloc will ingest. Raise it to match the input size your model needs.") cmd.Flags().StringVar(&schemaFlag, "schema", "", "tabular/time-series only: column types as col:TYPE,col:TYPE (e.g. age:INT,price:FLOAT). "+ "Default: inferred from the CSV (INT/FLOAT/VARCHAR).") @@ -326,6 +332,7 @@ type runDataIngestArgs struct { Namespace string Spec push.SpecArgs TargetSizeFlag string // raw --target-size; resolved after Discover (image) + MinSizeFlag string // raw --min-size; resolved after Discover (image) — #348 floor override SchemaFlag string // raw --schema; resolved or inferred after Discover (tabular) DryRun bool Overwrite bool @@ -616,6 +623,18 @@ collaborators can train against that table without ever seeing the raw files.`)) "resolution mismatch.\n", derr) } } + // Minimum-size floor override (#348): an explicit --min-size wins; + // otherwise the preview and the ingestor both fall back to the + // 32x32 default, so no spec field is emitted. This just plumbs the + // override to spec.file_options.min_size — the actual below-floor + // reject is previewed in runLocalPreflight (ValidateImages). + if a.MinSizeFlag != "" { + w, h, perr := push.ParseMinSize(a.MinSizeFlag) + if perr != nil { + return &exitError{code: 2, err: perr} + } + a.Spec.MinSize = []int{w, h} + } // Extension: every image must share one type, and the spec tells // the cluster which one to validate against (file_options.extension). // Without this the ingestor checked its .jpeg convention default and diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index 5f28b277..5ed27773 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -307,9 +307,9 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b prompted = true } if a.TargetSizeFlag == "" { - p.PromptHint("All images must share one resolution; the ingestor checks it (it won't resize). Blank = auto-detect from the first image. e.g. 224x224") - ans, err := pr.Input("Image resolution as WxH (blank = auto-detect from the first image)", - "all images must share it; the ingestor validates, it doesn't resize", "", + p.PromptHint("The resolution your images already are. tracebloc never resizes — it checks every image is exactly this size and rejects any that differ. Blank = read it from your first image. e.g. 224x224") + ans, err := pr.Input("Image resolution as WxH (blank = read it from your first image)", + "the size your images already are; tracebloc checks it, it never resizes", "", validateOptionalTargetSize) if err != nil { return prompted, err diff --git a/internal/push/detect.go b/internal/push/detect.go index 896d45c6..cdcf5444 100644 --- a/internal/push/detect.go +++ b/internal/push/detect.go @@ -44,6 +44,21 @@ func DetectImageSize(path string) (width, height int, err error) { // height]. Accepts "WxH" (the documented form, e.g. "512x512") and // "W,H" as a convenience. Both dimensions must be positive integers. func ParseTargetSize(s string) (width, height int, err error) { + return parseWxH("target size", s) +} + +// ParseMinSize parses a --min-size flag value into [width, height], +// the same WxH grammar as --target-size (#183). It plumbs to +// spec.file_options.min_size, the ingestor's minimum-image-size floor +// override (data-ingestors #348). +func ParseMinSize(s string) (width, height int, err error) { + return parseWxH("min size", s) +} + +// parseWxH parses a "WxH" (or "W,H") dimension pair, using kind in +// error messages so callers surface "target size …" / "min size …" +// verbatim. Both dimensions must be positive integers. +func parseWxH(kind, s string) (width, height int, err error) { sep := "x" if strings.Contains(s, ",") { sep = "," @@ -51,21 +66,21 @@ func ParseTargetSize(s string) (width, height int, err error) { parts := strings.Split(s, sep) if len(parts) != 2 { return 0, 0, fmt.Errorf( - "target size %q must be WxH (e.g. 512x512)", s) + "%s %q must be WxH (e.g. 512x512)", kind, s) } width, err = strconv.Atoi(strings.TrimSpace(parts[0])) if err != nil { return 0, 0, fmt.Errorf( - "target size %q: width is not an integer: %w", s, err) + "%s %q: width is not an integer: %w", kind, s, err) } height, err = strconv.Atoi(strings.TrimSpace(parts[1])) if err != nil { return 0, 0, fmt.Errorf( - "target size %q: height is not an integer: %w", s, err) + "%s %q: height is not an integer: %w", kind, s, err) } if width <= 0 || height <= 0 { return 0, 0, fmt.Errorf( - "target size %q: width and height must both be positive", s) + "%s %q: width and height must both be positive", kind, s) } return width, height, nil } diff --git a/internal/push/preflight.go b/internal/push/preflight.go index a2810eaf..d8943129 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -188,18 +188,28 @@ func CheckHasDataRows(path string) error { // ValidateImages previews the ingestor's ImageResolutionValidator // (image_validator.py): it opens EVERY image (header-only decode — cheap) -// and rejects zero-byte files, undecodable files, and any image whose -// resolution differs from the expected size (exact equality, zero -// tolerance — the ingestor validates, it does not resize). Previously the -// CLI decoded only the first image, so a single odd-sized or corrupt file -// failed in-cluster after the full upload (cli#72b/c). +// and rejects zero-byte files, undecodable files, images below the +// minimum-size floor, and any image whose resolution differs from the +// expected size (exact equality, zero tolerance — the ingestor validates, +// it does not resize). Previously the CLI decoded only the first image, so +// a single odd-sized or corrupt file failed in-cluster after the full +// upload (cli#72b/c). // // expectedW/expectedH of 0 skips the resolution comparison (the caller // couldn't establish a target size — the ingestor would then auto-detect // from its first file, which the CLI's detection already mirrors). -func ValidateImages(images []string, expectedW, expectedH int) error { +// +// minW/minH is the minimum-size floor (#348), mirroring the ingestor's +// _meets_min_size: an image is too small when EITHER side is below the +// floor; an image exactly at the floor passes. 0/0 disables the floor (a +// caller opt-out — production always passes push.MinImageSize or the +// --min-size override, so the floor is live end-to-end). The too-small +// check takes precedence over the resolution mismatch, exactly as +// data-ingestors #348 returns the too_small error before the +// target_size uniformity error. +func ValidateImages(images []string, expectedW, expectedH, minW, minH int) error { const maxListed = 5 - var broken, mismatched []string + var broken, tooSmall, mismatched []string for _, path := range images { name := filepath.Base(path) f, err := os.Open(path) @@ -217,11 +227,25 @@ func ValidateImages(images []string, expectedW, expectedH int) error { } continue } + if minW > 0 && minH > 0 && (cfg.Width < minW || cfg.Height < minH) { + tooSmall = append(tooSmall, + fmt.Sprintf("%s (%dx%d)", name, cfg.Width, cfg.Height)) + } if expectedW > 0 && expectedH > 0 && (cfg.Width != expectedW || cfg.Height != expectedH) { mismatched = append(mismatched, fmt.Sprintf("%s (%dx%d)", name, cfg.Width, cfg.Height)) } } + // Floor first: an image below the minimum size simply can't be trained + // on, so it's the most fundamental, actionable failure — data-ingestors + // #348 returns it ahead of the uniformity / target_size mismatch. + if len(tooSmall) > 0 { + return fmt.Errorf( + "%d image(s) are smaller than the %dx%d minimum: %s. The cluster rejects images below "+ + "this size after the upload — they're too small to train on. Provide larger images, "+ + "or lower the floor with --min-size, then re-run.", + len(tooSmall), minW, minH, TruncateList(tooSmall, maxListed)) + } if len(broken) > 0 { return fmt.Errorf( "%d image(s) can't be ingested: %s. The cluster rejects these after the upload — "+ @@ -688,7 +712,17 @@ func PreflightDataset(spec SpecArgs, layout *LocalLayout) (notes []string, probl if len(spec.TargetSize) == 2 { expW, expH = spec.TargetSize[0], spec.TargetSize[1] } - if err := ValidateImages(layout.Images, expW, expH); err != nil { + // Effective minimum-size floor (#348): the --min-size override + // (spec.MinSize) if the customer set one, else MinImageSize — the + // same default the ingestor applies when file_options.min_size is + // unset. Resolving the default HERE (not inside ValidateImages) + // mirrors the ingestor, whose __init__ defaults min_size to + // MIN_IMAGE_SIZE. + minW, minH := MinImageSize[0], MinImageSize[1] + if len(spec.MinSize) == 2 { + minW, minH = spec.MinSize[0], spec.MinSize[1] + } + if err := ValidateImages(layout.Images, expW, expH, minW, minH); err != nil { return nil, dataProblem(err) } if err := CheckHasDataRows(layout.LabelsCSV); err != nil { diff --git a/internal/push/preflight_test.go b/internal/push/preflight_test.go index ed546f4e..38ff928f 100644 --- a/internal/push/preflight_test.go +++ b/internal/push/preflight_test.go @@ -127,28 +127,77 @@ func TestValidateImages(t *testing.T) { zero := write("zero.png", nil) corrupt := write("corrupt.png", []byte("not an image at all")) - if err := ValidateImages([]string{good}, 8, 8); err != nil { + // minW/minH of 0 disables the floor so these decode/mismatch cases + // exercise the same behavior as before the #348 floor landed (the + // 8x8 / 4x4 fixtures are below the real 32x32 default). + if err := ValidateImages([]string{good}, 8, 8, 0, 0); err != nil { t.Errorf("valid image rejected: %v", err) } - if err := ValidateImages([]string{good, zero}, 8, 8); err == nil { + if err := ValidateImages([]string{good, zero}, 8, 8, 0, 0); err == nil { t.Fatal("zero-byte image must be rejected (cli#72b)") } else if !strings.Contains(err.Error(), "0 bytes") { t.Errorf("zero-byte diagnosis missing: %v", err) } - if err := ValidateImages([]string{good, corrupt}, 8, 8); err == nil { + if err := ValidateImages([]string{good, corrupt}, 8, 8, 0, 0); err == nil { t.Fatal("corrupt image must be rejected (cli#72b)") } - if err := ValidateImages([]string{good, odd}, 8, 8); err == nil { + if err := ValidateImages([]string{good, odd}, 8, 8, 0, 0); err == nil { t.Fatal("resolution mismatch must be rejected (cli#72c — the ingestor validates, it does not resize)") } else if !strings.Contains(err.Error(), "4x4") || !strings.Contains(err.Error(), "8x8") { t.Errorf("mismatch error must show both sizes: %v", err) } // 0x0 expectation skips the resolution comparison entirely. - if err := ValidateImages([]string{good, odd}, 0, 0); err != nil { + if err := ValidateImages([]string{good, odd}, 0, 0, 0, 0); err != nil { t.Errorf("no expected size → no resolution rejection: %v", err) } } +// TestValidateImagesMinSize covers the #348 minimum-size floor preview: +// an image below the floor is rejected (naming the file, its dimensions, +// and the floor); an image exactly at the floor passes; the floor takes +// precedence over a target_size mismatch; and it mirrors the ingestor's +// default (push.MinImageSize). +func TestValidateImagesMinSize(t *testing.T) { + dir := t.TempDir() + write := func(name string, w, h int) string { + p := filepath.Join(dir, name) + if err := os.WriteFile(p, pngBytes(t, w, h), 0o644); err != nil { + t.Fatal(err) + } + return p + } + minW, minH := MinImageSize[0], MinImageSize[1] // 32x32, mirrors data-ingestors #348 + + atFloor := write("at_floor.png", minW, minH) + aboveFloor := write("above.png", minW+16, minH+16) + belowW := write("below_w.png", minW-1, minH) // one side under → too small + tiny := write("tiny.png", 8, 8) // both sides under + + // At or above the floor passes (exact-floor image is accepted). + if err := ValidateImages([]string{atFloor, aboveFloor}, 0, 0, minW, minH); err != nil { + t.Errorf("at/above-floor images rejected: %v", err) + } + // One side below the floor → rejected, naming the file, its size, and the floor. + err := ValidateImages([]string{atFloor, belowW}, 0, 0, minW, minH) + if err == nil { + t.Fatal("below-floor image must be rejected (#348)") + } + for _, want := range []string{"below_w.png", "31x32", "32x32", "min-size"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("too-small error missing %q: %v", want, err) + } + } + // The floor takes precedence over a target_size mismatch: tiny is both + // below the floor AND != the 64x64 target, but the too-small message wins. + err = ValidateImages([]string{tiny}, 64, 64, minW, minH) + if err == nil { + t.Fatal("tiny image must be rejected") + } + if !strings.Contains(err.Error(), "minimum") { + t.Errorf("floor must take precedence over the mismatch message: %v", err) + } +} + func TestCrossCheckLabels(t *testing.T) { dir := t.TempDir() imgs := filepath.Join(dir, "images") diff --git a/internal/push/spec.go b/internal/push/spec.go index dbee3fe0..6dea95af 100644 --- a/internal/push/spec.go +++ b/internal/push/spec.go @@ -47,6 +47,20 @@ import ( // table-naming style anyway. var tableNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) +// MinImageSize is the absolute lower bound on image dimensions as +// [width, height] in pixels — the CLI's mirror of the ingestor's +// ImageResolutionValidator.MIN_IMAGE_SIZE (data-ingestors #348, RFC-0002 +// §12.9). Images with either side below this are rejected in-cluster as +// too small to train on, independently of the target_size uniformity +// check; the CLI previews that same reject locally (see ValidateImages) +// so a customer finds out before the ingest, not after. A per-model +// override travels in spec.file_options.min_size (SpecArgs.MinSize); when +// unset, this default applies on both sides. +// +// Keep this in lock-step with the upstream constant — it is the source of +// truth. Do NOT invent a different floor here (Principle 6). +var MinImageSize = [2]int{32, 32} + // MaxTableNameLength caps `--table` at 63 chars. Two hard limits // agree on this: // @@ -162,6 +176,15 @@ type SpecArgs struct { // re-introduce a swap. (See the inline comment in buildImage.) TargetSize []int + // MinSize, when len==2, holds the minimum acceptable image size as + // [W, H] — the override for the ingestor's minimum-image-size floor + // (data-ingestors #348). Emitted as spec.file_options.min_size. + // Empty (len 0) ⇒ omit, and both the CLI preview and the ingestor + // fall back to MinImageSize (32x32). Populated by the CLI from + // --min-size. Same [W, H] order as TargetSize — no swap. (Image + // categories only.) + MinSize []int + // Schema is the column→SQL-type map for tabular / time-series // categories (required by the schema for those). Populated by the // CLI from --schema or by inferring types from the CSV. Ignored @@ -285,6 +308,16 @@ func (a SpecArgs) buildImage(spec map[string]any, prefix string) { fileOptions["extension"] = a.Extension } + // Minimum-size floor override (#348). Emitted under file_options for + // every image category (the schema places min_size there — there is no + // top-level min_size like keypoint's target_size). [width, height] — + // same contract as target_size, no swap. Omitted when unset so the + // ingestor's MIN_IMAGE_SIZE default (32x32) applies, matching the + // CLI's own preview default. + if len(a.MinSize) == 2 { + fileOptions["min_size"] = []int{a.MinSize[0], a.MinSize[1]} + } + if a.Category == "keypoint_detection" { if len(a.TargetSize) == 2 { // Emitted as [width, height] — the schema's own description diff --git a/internal/push/spec_test.go b/internal/push/spec_test.go index 27c7cb1d..65e2becb 100644 --- a/internal/push/spec_test.go +++ b/internal/push/spec_test.go @@ -181,6 +181,65 @@ func TestBuild_WithTargetSize_PassesSchema(t *testing.T) { } } +// TestBuild_WithMinSize_PassesSchema pins the #183 plumbing: when the +// customer overrides the minimum-image-size floor (--min-size → +// SpecArgs.MinSize), Build emits spec.file_options.min_size as +// [width, height] and the result still validates against the embedded v1 +// schema (which learned about min_size in the #348 re-sync). +func TestBuild_WithMinSize_PassesSchema(t *testing.T) { + spec := SpecArgs{ + Table: "cats_dogs_train", + Category: "image_classification", + Intent: "train", + LabelColumn: "label", + MinSize: []int{64, 48}, // non-square, to also lock the [W, H] order + }.Build() + + fo, ok := spec["spec"].(map[string]any)["file_options"].(map[string]any) + if !ok { + t.Fatalf("spec.file_options missing/wrong type: %#v", spec["spec"]) + } + ms, ok := fo["min_size"].([]int) + if !ok || len(ms) != 2 || ms[0] != 64 || ms[1] != 48 { + t.Fatalf("spec.file_options.min_size = %#v, want [64 48] (width, height)", fo["min_size"]) + } + + specBytes, err := yaml.Marshal(spec) + if err != nil { + t.Fatalf("yaml.Marshal: %v", err) + } + v, err := schema.NewV1Validator() + if err != nil { + t.Fatalf("NewV1Validator: %v", err) + } + _, errs, parseErr := v.ValidateYAML(specBytes) + if parseErr != nil { + t.Fatalf("ValidateYAML parse error on our own output: %v\n%s", parseErr, specBytes) + } + if len(errs) != 0 { + t.Fatalf("spec with min_size failed schema validation: %s\nspec:\n%s", + schema.FormatErrors(errs), specBytes) + } +} + +// TestBuild_NoMinSize_OmitsMinSize: with no --min-size override, Build must +// NOT emit file_options.min_size — both the CLI preview and the ingestor +// then fall back to the shared 32x32 default (MinImageSize / MIN_IMAGE_SIZE). +func TestBuild_NoMinSize_OmitsMinSize(t *testing.T) { + spec := SpecArgs{ + Table: "t", + Category: "image_classification", + Intent: "train", + LabelColumn: "label", + TargetSize: []int{64, 64}, // emits a spec block, but min_size must be absent + }.Build() + if fo, ok := spec["spec"].(map[string]any)["file_options"].(map[string]any); ok { + if _, present := fo["min_size"]; present { + t.Errorf("Build() with no MinSize emitted file_options.min_size; want omitted") + } + } +} + // TestBuild_NoTargetSize_OmitsSpecBlock: when no resolution is set, // Build must NOT emit a spec block (the ingestor's per-category // default applies). Asserting the omission keeps the minimal-spec diff --git a/internal/push/testdata/parity/cases.json b/internal/push/testdata/parity/cases.json index b54b8729..8f0788b2 100644 --- a/internal/push/testdata/parity/cases.json +++ b/internal/push/testdata/parity/cases.json @@ -25,8 +25,8 @@ "csv": "data.csv", "label_column": "label", "cli_verdict": "reject", - "ingestor_verdict": "reject", - "note": "the stdlib header probe does not strip the BOM \u2014 false in-cluster rejection the CLI must preview (cli#71)" + "ingestor_verdict": "accept", + "note": "DELIBERATE divergence (surfaced by the #348 schema re-sync, which pinned the ref past data-ingestors #338): at THIS pinned ref the in-cluster tabular schema probe now strips the BOM, so the ingestor accepts. But the DEPLOYED ingestor (v0.5.7) predates #338 and still falsely rejects a BOM'd tabular CSV post-upload, so the CLI keeps previewing that rejection (CheckTabularBOM, cli#71). Drop this divergence once #338 ships to prod. Flagged for follow-up." }, { "name": "tabular-header-only", @@ -53,8 +53,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "accept", "ingestor_verdict": "accept", @@ -67,8 +67,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "accept", "ingestor_verdict": "accept", @@ -82,8 +82,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "reject", @@ -96,8 +96,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "accept", "ingestor_verdict": "accept", @@ -111,8 +111,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "reject", @@ -125,8 +125,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "reject", @@ -139,13 +139,27 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "reject", "note": "ImageResolutionValidator: exact equality, no resize (cli#72c)" }, + { + "name": "imgc-too-small", + "category": "image_classification", + "csv": "labels.csv", + "label_column": "label", + "extension": ".jpg", + "target_size": [ + 16, + 16 + ], + "cli_verdict": "reject", + "ingestor_verdict": "reject", + "note": "ImageResolutionValidator minimum-size floor (data-ingestors #348, RFC-0002 §12.9): 16x16 images are below the 32x32 default floor — both sides reject as too small to train on. target_size matches the images, so the floor (not a resolution mismatch or a diversity failure) is the trigger." + }, { "name": "imgc-header-only", "category": "image_classification", @@ -153,8 +167,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "reject", @@ -167,8 +181,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "accept", @@ -181,8 +195,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "reject", @@ -195,12 +209,12 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 4 + 64, + 32 ], "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]=[64,32] (both sides >= the 32x32 floor): 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 }, { @@ -210,12 +224,12 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 4, - 8 + 32, + 64 ], "cli_verdict": "reject", "ingestor_verdict": "reject", - "note": "the same 8\u00d74 images with target [4,8]: both sides must reject \u2014 proves the comparison is orientation-sensitive, not shape-normalized" + "note": "the same 64\u00d732 images with target [32,64]: both sides must reject \u2014 proves the comparison is orientation-sensitive, not shape-normalized" }, { "name": "imgc-dup-header", @@ -224,8 +238,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "accept", @@ -238,8 +252,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "accept", "ingestor_verdict": "accept", @@ -253,8 +267,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "accept", "ingestor_verdict": "accept", diff --git a/internal/push/testdata/parity/cases/imgc-bom-labels/images/a.jpg b/internal/push/testdata/parity/cases/imgc-bom-labels/images/a.jpg index 2c5a1e731107d9dcd1cdcd8a822612c6d01d9017..6520d3907647976516c4c9c3aaddbec125af8aef 100644 GIT binary patch delta 176 zcmey#(!x4HvYwfV350-v1&CNVSXo(ESh?8Q**JK(czC$ExVd@xgaml`1o^nR1;hjd zg+)X~MS1weCB#G|ghWI|{@-SBW@HBHVPWNDW#ts%<>nP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxuU00000 delta 164 zcmZo+{lPLpvYvyDjh&5^gPon7laqssM}(J$o0~^cNSI$lR!Uw@R!T-jK}AnpK}knh zMn=P2Q^&y2#Kc5i-O|>=$VSiD#OVKR24_Z2PEH^zXnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxuU00000 delta 164 zcmZo+{lPLpvYvyDjh&5^gPon7laqssM}(J$o0~^cNSI$lR!Uw@R!T-jK}AnpK}knh zMn=P2Q^&y2#Kc5i-O|>=$VSiD#OVKR24_Z2PEH^zXnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxuU00000 delta 164 zcmZo+{lPLpvYvyDjh&5^gPon7laqssM}(J$o0~^cNSI$lR!Uw@R!T-jK}AnpK}knh zMn=P2Q^&y2#Kc5i-O|>=$VSiD#OVKR24_Z2PEH^zXnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxuU00000 delta 164 zcmZo+{lPLpvYvyDjh&5^gPon7laqssM}(J$o0~^cNSI$lR!Uw@R!T-jK}AnpK}knh zMn=P2Q^&y2#Kc5i-O|>=$VSiD#OVKR24_Z2PEH^zXnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxuU00000 delta 164 zcmZo+{lPLpvYvyDjh&5^gPon7laqssM}(J$o0~^cNSI$lR!Uw@R!T-jK}AnpK}knh zMn=P2Q^&y2#Kc5i-O|>=$VSiD#OVKR24_Z2PEH^zXnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxuU00000 delta 164 zcmZo+{lPLpvYvyDjh&5^gPon7laqssM}(J$o0~^cNSI$lR!Uw@R!T-jK}AnpK}knh zMn=P2Q^&y2#Kc5i-O|>=$VSiD#OVKR24_Z2PEH^zXnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEH3G#7s3y28_ z3X6z}it_M_ONfa`2#JV_{J+iM%*YJX!@|nR%E~Fi%grl70^o4f*02GWMkN^Mx delta 164 zcmbQj`h#VHWIYEP8#@~-2Rl1ECnpCNj|eXhH#d)@kTAc9tdzW*tdxw5f{LEHf|8E1 zjEsi4rjCK3iHV84x}~j!k&T|QiP8Vt49<+4oSZz|JQBRT5=M$Libf;=eu&!t2N(o7 X7+4rMHZDKG$oVj1QP;;l3G#7s3y28_ z3X6z}it_M_ONfa`2#JV_{J+iM%*YJX!@|nR%E~Fi%grl70^o4f*02GWMkN^Mx delta 164 zcmbQj`h#VHWIYEP8#@~-2Rl1ECnpCNj|eXhH#d)@kTAc9tdzW*tdxw5f{LEHf|8E1 zjEsi4rjCK3iHV84x}~j!k&T|QiP8Vt49<+4oSZz|JQBRT5=M$Libf;=eu&!t2N(o7 X7+4rMHZDKG$oVj1QP;;l3G#7s3y28_ z3X6z}it_M_ONfa`2#JV_{J+iM%*YJX!@|nR%E~Fi%grl70^o4f*02GWMkN^Mx delta 164 zcmbQj`h#VHWIYEP8#@~-2Rl1ECnpCNj|eXhH#d)@kTAc9tdzW*tdxw5f{LEHf|8E1 zjEsi4rjCK3iHV84x}~j!k&T|QiP8Vt49<+4oSZz|JQBRT5=M$Libf;=eu&!t2N(o7 X7+4rMHZDKG$oVj1QP;;l3G#7s3y28_ z3X6z}it_M_ONfa`2#JV_{J+iM%*YJX!@|nR%E~Fi%grl70^o4f*02GWMkN^Mx delta 164 zcmbQj`h#VHWIYEP8#@~-2Rl1ECnpCNj|eXhH#d)@kTAc9tdzW*tdxw5f{LEHf|8E1 zjEsi4rjCK3iHV84x}~j!k&T|QiP8Vt49<+4oSZz|JQBRT5=M$Libf;=eu&!t2N(o7 X7+4rMHZDKG$oVj1QP;;lnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEHnP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEH3G#7s3y28_ z3X6z}it_M_ONfa`2#JV_{J+iM%*YJX!@|nR%E~Fi%grl70^oR^ Y6O5YAUo7hS$ai5;mj(kK%>4f*0Gkpa?EnA( delta 165 zcmbQi`jcgXWIYEP8#@~-2Rl1ECnpCNj|eXhH#d)@kTAc9tdzW*tdxw5f{LEHf|8E1 zjEsi4rjCK3iHV84x}~j!k&T|QiP8Vt49<+4oSZz|JQBRT5=M$Libf;=eu&!t2N(o7 Z7+4rsHZDKG$o08PW6_TO754vc0sw*fA!Yyo diff --git a/internal/push/testdata/parity/cases/imgc-too-small/images/a.jpg b/internal/push/testdata/parity/cases/imgc-too-small/images/a.jpg new file mode 100644 index 0000000000000000000000000000000000000000..7f0770f2afff74a46dd075cce4d33051fba53fa4 GIT binary patch literal 632 zcmex=``2_j6xdp@o1cgOJMMZh|#U;coyG6VInuyV4pa*FVB^NNrR z{vTivX!XqN1l2cOC(lau%ic3n%$}1|Xnp;}i+B-VC zCQY6)b=ve9GiNPYykzOJeA&aSFc^aar4&0 zM~|O8efIpt%U2&ieg5+G+xH(oe}VkP$iNKo7LbH^49#DHKz}i@urRZ*gZ#zFR1U<< zf-J0xhHOHPf$WKe!b(Ps93oB=7j8VrscandK{To8BA1wo$wSqTAg_UaMx4i*$nqK7 eV+eoUV&GwB1V$dSAcH-_^B0S{KJs1ue-i+7``2_j6xdp@o1cgOJMMZh|#U;coyG6VInuyV4pa*FVB^NNrR z{vTivX!XqN1l2cOC(lau%ic3n%$}1|Xnp;}i+B-VC zCQY6)b=ve9GiNPYykzOJeA&aSFc^aar4&0 zM~|O8efIpt%U2&ieg5+G+xH(oe}VkP$iNKo7LbH^49#DHKz}i@urRZ*gZ#zFR1U<< zf-J0xhHOHPf$WKe!b(Ps93oB=7j8VrscandK{To8BA1wo$wSqTAg_UaMx4i*$nqK7 eV+eoUV&GwB1V$dSAcH-_^B0S{KJs1ue-i+7nP30q{fA{y)GV$ibk%ps;cI Y2}aT9FBWxu=$VSiD#OVKR24_Z2PEH.1' by the parser and the schema maps onto the wrong column." diff --git a/internal/schema/ingest.v1.json b/internal/schema/ingest.v1.json index ea371d1d..b6f6bef7 100644 --- a/internal/schema/ingest.v1.json +++ b/internal/schema/ingest.v1.json @@ -204,6 +204,13 @@ "maxItems": 2, "description": "[width, height]. Image categories only. Default [512, 512]. The order matches PIL.Image.size and what ImageResolutionValidator expects." }, + "min_size": { + "type": "array", + "items": { "type": "integer", "minimum": 1 }, + "minItems": 2, + "maxItems": 2, + "description": "[width, height] absolute minimum image size (#348). Images with either side below this are rejected as too small to train on. Image categories only. Defaults to [32, 32] (ImageResolutionValidator.MIN_IMAGE_SIZE) when unset; override per-model to match the model-zoo input requirement." + }, "extension": { "type": "string", "enum": [".jpg", ".jpeg", ".png", ".txt", ".text", ".xml"], diff --git a/scripts/.data-ingestors-ref b/scripts/.data-ingestors-ref index c441fc2c..843291e2 100644 --- a/scripts/.data-ingestors-ref +++ b/scripts/.data-ingestors-ref @@ -9,4 +9,4 @@ # # Format: the first non-comment, non-blank line is the ref (a full commit SHA # preferred; a branch name works but reintroduces floating drift). -0de1f148f9f19c8838d275ab9e5295ae224385c2 +efaeb07185c42556f833e876cb17791f30f4916d From 7cc5337b8e0faab02c9453aa04a596042858c602 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Thu, 9 Jul 2026 20:10:44 +0200 Subject: [PATCH 2/4] fix(data ingest): gate the image size floor on the deployed ingestor + close the min-size parity gap (#183) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the #183 min-image-size work: - Deployment parity (was the inverse of the tabular-BOM handling): the 32x32 floor lives in data-ingestors only on develop (di#348/#356); the DEPLOYED ingestor (v0.5.7/v0.6.0) has no floor and ingests small images fine. The preview no longer applies the default floor on its own — a default block would exit-3 an ingest the live cluster accepts. It now previews the floor ONLY when the customer set --min-size (their own declared requirement). Flip imgc-too-small to a flagged deliberate divergence; restore default-on once di#348 reaches prod. - Message: the too-small reject no longer asserts "the cluster rejects images below this size" (false for the deployed cluster) — it speaks to the floor the customer set with --min-size. - Flag help: --min-size no longer calls 32x32 "the smallest tracebloc will ingest" (only-raise), which contradicted the reject's "lower the floor with --min-size" and the feature (16x16 is accepted end-to-end). Reworded to raise-or-lower with an honest "no local size check" default. - Parity harness: min_size was representable on neither side, so the headline override was cross-checked by neither. Plumb it through the Go parityCase + SpecArgs and gen-validator-goldens.py, and add imgc-min-size-override — 24x40 images with an explicit [16,32] floor that the DEFAULT 32x32 would reject, pinning the override is read (not the default) and its [W,H] orientation is honored, on both sides. Goldens regenerated against the pinned ref (efaeb07); only the new case is added, imgc-too-small's golden is unchanged. Co-Authored-By: Claude Opus 4.8 --- internal/cli/data.go | 20 +++++---- internal/push/parity_golden_test.go | 2 + internal/push/preflight.go | 38 +++++++++++------- internal/push/spec.go | 12 ++++-- internal/push/testdata/parity/cases.json | 23 ++++++++++- .../cases/imgc-min-size-override/images/a.jpg | Bin 0 -> 653 bytes .../cases/imgc-min-size-override/images/b.jpg | Bin 0 -> 653 bytes .../cases/imgc-min-size-override/labels.csv | 3 ++ internal/push/testdata/parity/goldens.json | 12 ++++++ scripts/gen-validator-goldens.py | 6 +++ 10 files changed, 87 insertions(+), 29 deletions(-) create mode 100644 internal/push/testdata/parity/cases/imgc-min-size-override/images/a.jpg create mode 100644 internal/push/testdata/parity/cases/imgc-min-size-override/images/b.jpg create mode 100644 internal/push/testdata/parity/cases/imgc-min-size-override/labels.csv diff --git a/internal/cli/data.go b/internal/cli/data.go index e4e098f2..f02bd846 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -280,10 +280,11 @@ Exit codes: cmd.Flags().StringVar(&targetSize, "target-size", "", "image tasks only: the resolution your images already are, as WxH (e.g. 512x512). tracebloc never "+ "resizes — it checks every image is exactly this size and rejects any that differ. Default: "+ - "read from your first image. Images below the --min-size floor are rejected regardless.") + "read from your first image.") cmd.Flags().StringVar(&minSize, "min-size", "", - "image tasks only: reject images smaller than WxH (e.g. 64x64). Default: 32x32, the smallest "+ - "tracebloc will ingest. Raise it to match the input size your model needs.") + "image tasks only: reject images smaller than WxH before the ingest (e.g. 64x64). Set it to the "+ + "smallest size your model can train on — raise or lower it freely. Default: unset (no local "+ + "size check).") cmd.Flags().StringVar(&schemaFlag, "schema", "", "tabular/time-series only: column types as col:TYPE,col:TYPE (e.g. age:INT,price:FLOAT). "+ "Default: inferred from the CSV (INT/FLOAT/VARCHAR).") @@ -623,11 +624,14 @@ collaborators can train against that table without ever seeing the raw files.`)) "resolution mismatch.\n", derr) } } - // Minimum-size floor override (#348): an explicit --min-size wins; - // otherwise the preview and the ingestor both fall back to the - // 32x32 default, so no spec field is emitted. This just plumbs the - // override to spec.file_options.min_size — the actual below-floor - // reject is previewed in runLocalPreflight (ValidateImages). + // Minimum-size floor override (#348): plumb an explicit --min-size to + // spec.file_options.min_size. When unset, no spec field is emitted, so + // the ingestor applies its own default (none on the deployed + // v0.5.7/v0.6.0; 32x32 on develop post-#348) — and the local preview + // applies NO floor either (PreflightDataset only previews the floor + // when --min-size is set, so it never rejects an ingest the live + // cluster accepts). The below-floor reject is previewed in + // runLocalPreflight (ValidateImages). if a.MinSizeFlag != "" { w, h, perr := push.ParseMinSize(a.MinSizeFlag) if perr != nil { diff --git a/internal/push/parity_golden_test.go b/internal/push/parity_golden_test.go index 186db758..9fe6efff 100644 --- a/internal/push/parity_golden_test.go +++ b/internal/push/parity_golden_test.go @@ -36,6 +36,7 @@ type parityCase struct { LabelColumn string `json:"label_column"` Extension string `json:"extension"` TargetSize []int `json:"target_size"` + MinSize []int `json:"min_size"` Schema map[string]string `json:"schema"` CLIVerdict string `json:"cli_verdict"` IngestorVerdict string `json:"ingestor_verdict"` @@ -143,6 +144,7 @@ func runGoPreflight(t *testing.T, c parityCase) string { LabelColumn: c.LabelColumn, Extension: c.Extension, TargetSize: c.TargetSize, + MinSize: c.MinSize, } if IsTabular(c.Category) { // Mirror runDataIngest: an explicit schema (the --schema flow) wins, diff --git a/internal/push/preflight.go b/internal/push/preflight.go index d8943129..4b884d04 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -201,11 +201,13 @@ func CheckHasDataRows(path string) error { // // minW/minH is the minimum-size floor (#348), mirroring the ingestor's // _meets_min_size: an image is too small when EITHER side is below the -// floor; an image exactly at the floor passes. 0/0 disables the floor (a -// caller opt-out — production always passes push.MinImageSize or the -// --min-size override, so the floor is live end-to-end). The too-small -// check takes precedence over the resolution mismatch, exactly as -// data-ingestors #348 returns the too_small error before the +// floor; an image exactly at the floor passes. 0/0 disables the floor. +// PreflightDataset passes a non-zero floor ONLY when the customer set +// --min-size — it does NOT default to MinImageSize, because the deployed +// ingestor has no floor yet (see the PreflightDataset image branch), so a +// default block would reject an ingest the live cluster accepts. The +// too-small check takes precedence over the resolution mismatch, exactly +// as data-ingestors #348 returns the too_small error before the // target_size uniformity error. func ValidateImages(images []string, expectedW, expectedH, minW, minH int) error { const maxListed = 5 @@ -241,9 +243,8 @@ func ValidateImages(images []string, expectedW, expectedH, minW, minH int) error // #348 returns it ahead of the uniformity / target_size mismatch. if len(tooSmall) > 0 { return fmt.Errorf( - "%d image(s) are smaller than the %dx%d minimum: %s. The cluster rejects images below "+ - "this size after the upload — they're too small to train on. Provide larger images, "+ - "or lower the floor with --min-size, then re-run.", + "%d image(s) are smaller than the %dx%d minimum you set with --min-size: %s. "+ + "Provide larger images, or lower the floor with --min-size, then re-run.", len(tooSmall), minW, minH, TruncateList(tooSmall, maxListed)) } if len(broken) > 0 { @@ -712,13 +713,20 @@ func PreflightDataset(spec SpecArgs, layout *LocalLayout) (notes []string, probl if len(spec.TargetSize) == 2 { expW, expH = spec.TargetSize[0], spec.TargetSize[1] } - // Effective minimum-size floor (#348): the --min-size override - // (spec.MinSize) if the customer set one, else MinImageSize — the - // same default the ingestor applies when file_options.min_size is - // unset. Resolving the default HERE (not inside ValidateImages) - // mirrors the ingestor, whose __init__ defaults min_size to - // MIN_IMAGE_SIZE. - minW, minH := MinImageSize[0], MinImageSize[1] + // Minimum-size floor (#348). The floor lives in data-ingestors only + // on develop (di#348/#356); the DEPLOYED ingestor (v0.5.7/v0.6.0) has + // no floor and ingests small images fine. So the preview must NOT + // apply the 32x32 default on its own — a default block would reject an + // ingest the live cluster accepts, the inverse of the tabular-BOM + // block (whose reject mirrors a real deployed rejection). Apply the + // floor ONLY when the customer explicitly set --min-size (spec.MinSize) + // — their own declared requirement, honored locally regardless of the + // cluster. Once di#348 reaches prod, default this to MinImageSize and + // flip the imgc-too-small parity case so the floor is previewed by + // default. The emit side already matches: it omits file_options.min_size + // when unset, letting whichever ingestor is deployed apply its own + // default (none today; MinImageSize post-#348). + minW, minH := 0, 0 if len(spec.MinSize) == 2 { minW, minH = spec.MinSize[0], spec.MinSize[1] } diff --git a/internal/push/spec.go b/internal/push/spec.go index 6dea95af..62a07deb 100644 --- a/internal/push/spec.go +++ b/internal/push/spec.go @@ -52,10 +52,14 @@ var tableNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) // ImageResolutionValidator.MIN_IMAGE_SIZE (data-ingestors #348, RFC-0002 // §12.9). Images with either side below this are rejected in-cluster as // too small to train on, independently of the target_size uniformity -// check; the CLI previews that same reject locally (see ValidateImages) -// so a customer finds out before the ingest, not after. A per-model -// override travels in spec.file_options.min_size (SpecArgs.MinSize); when -// unset, this default applies on both sides. +// check. A per-model override travels in spec.file_options.min_size +// (SpecArgs.MinSize); when unset, the ingestor applies this default (on +// develop — see below). The CLI preview does NOT default to it: it only +// previews the floor when the customer set --min-size, because the DEPLOYED +// ingestor (v0.5.7/v0.6.0) has no floor yet, so a default local block would +// reject an ingest the live cluster accepts (see PreflightDataset). This +// constant is still the mirror of the upstream default — once di#348 reaches +// prod, PreflightDataset should default the preview floor to it. // // Keep this in lock-step with the upstream constant — it is the source of // truth. Do NOT invent a different floor here (Principle 6). diff --git a/internal/push/testdata/parity/cases.json b/internal/push/testdata/parity/cases.json index 8f0788b2..fc61d9fc 100644 --- a/internal/push/testdata/parity/cases.json +++ b/internal/push/testdata/parity/cases.json @@ -156,9 +156,28 @@ 16, 16 ], - "cli_verdict": "reject", + "cli_verdict": "accept", "ingestor_verdict": "reject", - "note": "ImageResolutionValidator minimum-size floor (data-ingestors #348, RFC-0002 §12.9): 16x16 images are below the 32x32 default floor — both sides reject as too small to train on. target_size matches the images, so the floor (not a resolution mismatch or a diversity failure) is the trigger." + "note": "DELIBERATE divergence (deployment parity, #348): 16x16 images are below the 32x32 default floor, so the pinned ingestor (develop, di#348/#356) rejects. But the CLI must NOT block by default — the DEPLOYED ingestor (v0.5.7/v0.6.0) has NO floor and ingests these fine, so a default local reject would block an ingest the live cluster accepts (the inverse of tabular-bom, whose reject mirrors a real deployed rejection). The CLI previews the floor ONLY when --min-size is set (see imgc-min-size-override). Flip this to reject and default the preview floor once di#348 ships to prod. Flagged for follow-up." + }, + { + "name": "imgc-min-size-override", + "category": "image_classification", + "csv": "labels.csv", + "label_column": "label", + "extension": ".jpg", + "target_size": [ + 24, + 40 + ], + "min_size": [ + 16, + 32 + ], + "cli_verdict": "accept", + "ingestor_verdict": "accept", + "note": "the --min-size override, cross-checked end-to-end (#348; closes the parity gap where min_size was representable on neither side). 24x40 images (W,H) with an explicit min_size [16,32] pass on BOTH sides — but the DEFAULT 32x32 floor would reject them (W=24<32), so this proves the override is actually read (not silently the default) and its [W,H] orientation is honored (a swapped [32,16] would reject on W=24<32). target_size matches the images, so the floor override is the sole discriminator.", + "value_parity": true }, { "name": "imgc-header-only", diff --git a/internal/push/testdata/parity/cases/imgc-min-size-override/images/a.jpg b/internal/push/testdata/parity/cases/imgc-min-size-override/images/a.jpg new file mode 100644 index 0000000000000000000000000000000000000000..6fb2d28f4f0a3da45322cccedc8b4206e89a28da GIT binary patch literal 653 zcmex=^(PF6}rMnOeST|r4lSw=>~TvNxu(8R<c1}I=;VrF4wW9Q)H;sz?% zD!{d!pzFb!U9xX3zTPI5o8roG<0MW4oqZMDikqloVbuf*=gfJ(V&YTRE(2~ znmD<{#3dx9RMpfqG__1j&CD$#!xN1~T_61~Gj(Y!K*#+5Zvp^bsMbXQ literal 0 HcmV?d00001 diff --git a/internal/push/testdata/parity/cases/imgc-min-size-override/images/b.jpg b/internal/push/testdata/parity/cases/imgc-min-size-override/images/b.jpg new file mode 100644 index 0000000000000000000000000000000000000000..58494a06b7f86fca1eecf96f80e67734c34e022d GIT binary patch literal 653 zcmex=^(PF6}rMnOeST|r4lSw=>~TvNxu(8R<c1}I=;VrF4wW9Q)H;sz?% zD!{d!pzFb!U9xX3zTPI5o8roG<0MW4oqZMDikqloVbuf*=gfJ(V&YTRE(2~ znmD<{#3dx9RMpfqG__1j&CD$#!)GpyMf-g$4s~fPU_!(E|8D{SPmR^Y literal 0 HcmV?d00001 diff --git a/internal/push/testdata/parity/cases/imgc-min-size-override/labels.csv b/internal/push/testdata/parity/cases/imgc-min-size-override/labels.csv new file mode 100644 index 00000000..aa98ba50 --- /dev/null +++ b/internal/push/testdata/parity/cases/imgc-min-size-override/labels.csv @@ -0,0 +1,3 @@ +image_id,label +a.jpg,cat +b.jpg,dog diff --git a/internal/push/testdata/parity/goldens.json b/internal/push/testdata/parity/goldens.json index 635880ff..677290ce 100644 --- a/internal/push/testdata/parity/goldens.json +++ b/internal/push/testdata/parity/goldens.json @@ -77,6 +77,18 @@ ], "verdict": "reject" }, + "imgc-min-size-override": { + "errors": [], + "values": { + "classes": [ + "cat", + "dog" + ], + "resolved_label": "label", + "row_count": 2 + }, + "verdict": "accept" + }, "imgc-missing-file": { "errors": [], "verdict": "accept" diff --git a/scripts/gen-validator-goldens.py b/scripts/gen-validator-goldens.py index 99ed3b20..02ef6893 100644 --- a/scripts/gen-validator-goldens.py +++ b/scripts/gen-validator-goldens.py @@ -134,6 +134,12 @@ def run_case(case): options["extension"] = case["extension"] if case.get("target_size"): options["target_size"] = case["target_size"] + if case.get("min_size"): + # The --min-size floor override (#348), cross-checked end-to-end: + # the image factory reads options["min_size"] into + # ImageResolutionValidator, so a per-case override drives the REAL + # validator the same way the Go preview drives SpecArgs.MinSize. + options["min_size"] = case["min_size"] if case["category"].startswith(("tabular", "time_")): # An explicit per-case schema (mirroring --schema) wins; else # infer — BOTH sides of the harness use the same source so From f70ec4e275b28bdd8ac31ff1e6ec064d74dfd47d Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 10 Jul 2026 12:24:05 +0500 Subject: [PATCH 3/4] fix(data ingest): reject image-only flags on non-image tasks + surface --min-size (#206 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups from the #206 code review: - --target-size / --min-size are image-only, but were parsed only inside the image branch, so on a tabular/text task the flag value (even a malformed one) was silently dropped. Reject them explicitly at the category gate with an actionable exit-2 message. Adds a table-driven test. - renderReview now echoes a set --min-size in the interactive confirm summary, so a mistyped floor is caught before touching data. - Drop dangling citations in the MinImageSize doc-comment ("RFC-0002 §12.9" / "Principle 6" — neither exists; RFC-0002 has 5 principles and §12 stops at §12.8). Keep the real source (data-ingestors #348). Co-Authored-By: Claude Opus 4.8 --- internal/cli/coverage_test.go | 39 +++++++++++++++++++++++++++++++++++ internal/cli/data.go | 18 ++++++++++++++++ internal/cli/interactive.go | 6 ++++++ internal/push/spec.go | 6 +++--- 4 files changed, 66 insertions(+), 3 deletions(-) diff --git a/internal/cli/coverage_test.go b/internal/cli/coverage_test.go index 586ee645..7e65bc5f 100644 --- a/internal/cli/coverage_test.go +++ b/internal/cli/coverage_test.go @@ -194,6 +194,45 @@ func TestRunDatasetPush_OutputJSONEarlyFailureEmitsJSON(t *testing.T) { } } +// TestRunDataIngest_ImageOnlyFlagsRejectedOnNonImage: --target-size and +// --min-size describe image resolution, so on a tabular/text task they +// must fail fast (exit 2) with a clear message rather than being parsed +// only inside the image branch — where the value, even a malformed one, +// was silently dropped (#206 review). +func TestRunDataIngest_ImageOnlyFlagsRejectedOnNonImage(t *testing.T) { + cases := []struct { + name string + mutate func(*runDataIngestArgs) + }{ + {"target-size on tabular", func(a *runDataIngestArgs) { a.TargetSizeFlag = "64x64" }}, + {"min-size on tabular", func(a *runDataIngestArgs) { a.MinSizeFlag = "32x32" }}, + {"malformed min-size on tabular", func(a *runDataIngestArgs) { a.MinSizeFlag = "garbage" }}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var human bytes.Buffer + a := runDataIngestArgs{ + LocalPath: "./x", + Spec: push.SpecArgs{ + Table: "t", Category: "tabular_classification", + Intent: "train", LabelColumn: "y", + }, + Printer: ui.New(&human, ui.WithColor(false)), + } + c.mutate(&a) + err := runDataIngest(context.Background(), &human, &human, a) + + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("err = %v, want *exitError code 2", err) + } + if !strings.Contains(ee.Error(), "image tasks only") { + t.Errorf("error should explain the flag is image-only; got: %v", ee) + } + }) + } +} + // TestExpandHome covers the #37 fix: a leading ~ / ~/… resolves under // $HOME, while relative, absolute, and empty paths pass through // untouched (the case that bit the interactive prompt — the shell diff --git a/internal/cli/data.go b/internal/cli/data.go index f02bd846..e5b6ef79 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -518,6 +518,24 @@ collaborators can train against that table without ever seeing the raw files.`)) a.Spec.Category, push.SupportedCategoriesList())} } + // Image-only flags. --target-size / --min-size describe image + // resolution, so they're meaningless on a tabular / text task. + // Reject them explicitly here: without this guard they'd be parsed + // only inside the image branch below, so on a non-image task the + // value — even a malformed one — was silently dropped with no error. + if !push.IsImage(a.Spec.Category) { + for _, f := range []struct{ name, val string }{ + {"--target-size", a.TargetSizeFlag}, + {"--min-size", a.MinSizeFlag}, + } { + if f.val != "" { + return &exitError{code: 2, err: fmt.Errorf( + "%s is image tasks only; it doesn't apply to task %q", + f.name, a.Spec.Category)} + } + } + } + // 3. Walk the local directory FIRST (local "fail fast"), dispatched // by category family. Image categories expect labels.csv + // images/; tabular / time-series categories expect a single diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index 5ed27773..07a5126d 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -402,6 +402,12 @@ func renderReview(p *ui.Printer, a *runDataIngestArgs) { case push.IsImage(a.Spec.Category): p.Field("resolution", "auto-detect") } + // Only shown when set — --min-size is opt-in with no local default, + // so there's nothing to echo otherwise. Surfacing it lets a mistyped + // floor (e.g. 640x640 for 64x64) be caught at the confirm gate. + if a.MinSizeFlag != "" { + p.Field("min size", a.MinSizeFlag) + } switch { case a.SchemaFlag != "": p.Field("schema", a.SchemaFlag) diff --git a/internal/push/spec.go b/internal/push/spec.go index 62a07deb..73dafcd6 100644 --- a/internal/push/spec.go +++ b/internal/push/spec.go @@ -49,8 +49,8 @@ var tableNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) // MinImageSize is the absolute lower bound on image dimensions as // [width, height] in pixels — the CLI's mirror of the ingestor's -// ImageResolutionValidator.MIN_IMAGE_SIZE (data-ingestors #348, RFC-0002 -// §12.9). Images with either side below this are rejected in-cluster as +// ImageResolutionValidator.MIN_IMAGE_SIZE (data-ingestors #348). +// Images with either side below this are rejected in-cluster as // too small to train on, independently of the target_size uniformity // check. A per-model override travels in spec.file_options.min_size // (SpecArgs.MinSize); when unset, the ingestor applies this default (on @@ -62,7 +62,7 @@ var tableNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) // prod, PreflightDataset should default the preview floor to it. // // Keep this in lock-step with the upstream constant — it is the source of -// truth. Do NOT invent a different floor here (Principle 6). +// truth. Do NOT invent a different floor here. var MinImageSize = [2]int{32, 32} // MaxTableNameLength caps `--table` at 63 chars. Two hard limits From fd4688e4e86c3982871d06b34dbb239dd2570910 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 10 Jul 2026 12:33:47 +0500 Subject: [PATCH 4/4] test(data ingest): use a real path in image-only-flag guard test The test used LocalPath "./x"; on the branch tip the guard fired before any path check, but CI runs the PR merged into develop, which added an earlier dataset-path stat (#181) that rejects a nonexistent path with exit 3 before the guard's exit 2. Point LocalPath at a t.TempDir() so the stat passes and the image-only guard is what fires. Verified against a local merge with origin/develop. Co-Authored-By: Claude Opus 4.8 --- internal/cli/coverage_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/cli/coverage_test.go b/internal/cli/coverage_test.go index 7e65bc5f..1199679a 100644 --- a/internal/cli/coverage_test.go +++ b/internal/cli/coverage_test.go @@ -208,11 +208,15 @@ func TestRunDataIngest_ImageOnlyFlagsRejectedOnNonImage(t *testing.T) { {"min-size on tabular", func(a *runDataIngestArgs) { a.MinSizeFlag = "32x32" }}, {"malformed min-size on tabular", func(a *runDataIngestArgs) { a.MinSizeFlag = "garbage" }}, } + // A real, existing path so the earlier dataset-path stat passes and + // the image-only guard is what actually fires (the path check runs + // before the guard). + dir := t.TempDir() for _, c := range cases { t.Run(c.name, func(t *testing.T) { var human bytes.Buffer a := runDataIngestArgs{ - LocalPath: "./x", + LocalPath: dir, Spec: push.SpecArgs{ Table: "t", Category: "tabular_classification", Intent: "train", LabelColumn: "y",