diff --git a/internal/cli/coverage_test.go b/internal/cli/coverage_test.go index 586ee645..1199679a 100644 --- a/internal/cli/coverage_test.go +++ b/internal/cli/coverage_test.go @@ -194,6 +194,49 @@ 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" }}, + } + // 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: dir, + 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 89bdf348..e5b6ef79 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,13 @@ 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.") + cmd.Flags().StringVar(&minSize, "min-size", "", + "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).") @@ -326,6 +333,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 @@ -510,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 @@ -616,6 +642,21 @@ collaborators can train against that table without ever seeing the raw files.`)) "resolution mismatch.\n", derr) } } + // 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 { + 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..07a5126d 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 @@ -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/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/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 a2810eaf..4b884d04 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -188,18 +188,30 @@ 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. +// 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 - var broken, mismatched []string + var broken, tooSmall, mismatched []string for _, path := range images { name := filepath.Base(path) f, err := os.Open(path) @@ -217,11 +229,24 @@ 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 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 { return fmt.Errorf( "%d image(s) can't be ingested: %s. The cluster rejects these after the upload — "+ @@ -688,7 +713,24 @@ 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 { + // 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] + } + 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..73dafcd6 100644 --- a/internal/push/spec.go +++ b/internal/push/spec.go @@ -47,6 +47,24 @@ 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). +// 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 +// 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. +var MinImageSize = [2]int{32, 32} + // MaxTableNameLength caps `--table` at 63 chars. Two hard limits // agree on this: // @@ -162,6 +180,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 +312,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..fc61d9fc 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,46 @@ "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": "accept", + "ingestor_verdict": "reject", + "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", "category": "image_classification", @@ -153,8 +186,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "reject", @@ -167,8 +200,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "accept", @@ -181,8 +214,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "reject", @@ -195,12 +228,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 +243,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 +257,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "reject", "ingestor_verdict": "accept", @@ -238,8 +271,8 @@ "label_column": "label", "extension": ".jpg", "target_size": [ - 8, - 8 + 32, + 32 ], "cli_verdict": "accept", "ingestor_verdict": "accept", @@ -253,8 +286,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 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-bom-labels/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-bom-labels/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-bom-labels/images/b.jpg b/internal/push/testdata/parity/cases/imgc-bom-labels/images/b.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-bom-labels/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-bom-labels/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-corrupt/images/a.jpg b/internal/push/testdata/parity/cases/imgc-corrupt/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-corrupt/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-corrupt/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-dotted-stem/images/b.jpg b/internal/push/testdata/parity/cases/imgc-dotted-stem/images/b.jpg index c6b07d9d..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-dotted-stem/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-dotted-stem/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-dotted-stem/images/photo.2024.jpg b/internal/push/testdata/parity/cases/imgc-dotted-stem/images/photo.2024.jpg index c6b07d9d..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-dotted-stem/images/photo.2024.jpg and b/internal/push/testdata/parity/cases/imgc-dotted-stem/images/photo.2024.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-dup-header/images/a.jpg b/internal/push/testdata/parity/cases/imgc-dup-header/images/a.jpg index c6b07d9d..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-dup-header/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-dup-header/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-dup-header/images/b.jpg b/internal/push/testdata/parity/cases/imgc-dup-header/images/b.jpg index c6b07d9d..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-dup-header/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-dup-header/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-empty-label/images/a.jpg b/internal/push/testdata/parity/cases/imgc-empty-label/images/a.jpg index c6b07d9d..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-empty-label/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-empty-label/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-empty-label/images/b.jpg b/internal/push/testdata/parity/cases/imgc-empty-label/images/b.jpg index c6b07d9d..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-empty-label/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-empty-label/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-header-only/images/a.jpg b/internal/push/testdata/parity/cases/imgc-header-only/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-header-only/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-header-only/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-label-case/images/a.jpg b/internal/push/testdata/parity/cases/imgc-label-case/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-label-case/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-label-case/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-label-case/images/b.jpg b/internal/push/testdata/parity/cases/imgc-label-case/images/b.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-label-case/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-label-case/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-label-missing/images/a.jpg b/internal/push/testdata/parity/cases/imgc-label-missing/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-label-missing/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-label-missing/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-label-uniform/images/a.jpg b/internal/push/testdata/parity/cases/imgc-label-uniform/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-label-uniform/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-label-uniform/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-label-uniform/images/b.jpg b/internal/push/testdata/parity/cases/imgc-label-uniform/images/b.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-label-uniform/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-label-uniform/images/b.jpg differ 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 00000000..6fb2d28f Binary files /dev/null and b/internal/push/testdata/parity/cases/imgc-min-size-override/images/a.jpg differ 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 00000000..58494a06 Binary files /dev/null and b/internal/push/testdata/parity/cases/imgc-min-size-override/images/b.jpg differ 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/cases/imgc-missing-file/images/a.jpg b/internal/push/testdata/parity/cases/imgc-missing-file/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-missing-file/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-missing-file/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-nonsquare-swapped/images/a.jpg b/internal/push/testdata/parity/cases/imgc-nonsquare-swapped/images/a.jpg index 4b5275be..0c7b0ff7 100644 Binary files a/internal/push/testdata/parity/cases/imgc-nonsquare-swapped/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-nonsquare-swapped/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-nonsquare-swapped/images/b.jpg b/internal/push/testdata/parity/cases/imgc-nonsquare-swapped/images/b.jpg index 4b5275be..0c7b0ff7 100644 Binary files a/internal/push/testdata/parity/cases/imgc-nonsquare-swapped/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-nonsquare-swapped/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-nonsquare/images/a.jpg b/internal/push/testdata/parity/cases/imgc-nonsquare/images/a.jpg index 4b5275be..0c7b0ff7 100644 Binary files a/internal/push/testdata/parity/cases/imgc-nonsquare/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-nonsquare/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-nonsquare/images/b.jpg b/internal/push/testdata/parity/cases/imgc-nonsquare/images/b.jpg index 4b5275be..0c7b0ff7 100644 Binary files a/internal/push/testdata/parity/cases/imgc-nonsquare/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-nonsquare/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-ok/images/a.jpg b/internal/push/testdata/parity/cases/imgc-ok/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-ok/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-ok/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-ok/images/b.jpg b/internal/push/testdata/parity/cases/imgc-ok/images/b.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-ok/images/b.jpg and b/internal/push/testdata/parity/cases/imgc-ok/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-res-mismatch/images/a.jpg b/internal/push/testdata/parity/cases/imgc-res-mismatch/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-res-mismatch/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-res-mismatch/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-res-mismatch/images/odd.jpg b/internal/push/testdata/parity/cases/imgc-res-mismatch/images/odd.jpg index 3a59e2e8..66f02eed 100644 Binary files a/internal/push/testdata/parity/cases/imgc-res-mismatch/images/odd.jpg and b/internal/push/testdata/parity/cases/imgc-res-mismatch/images/odd.jpg differ 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 00000000..7f0770f2 Binary files /dev/null and b/internal/push/testdata/parity/cases/imgc-too-small/images/a.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-too-small/images/b.jpg b/internal/push/testdata/parity/cases/imgc-too-small/images/b.jpg new file mode 100644 index 00000000..7f0770f2 Binary files /dev/null and b/internal/push/testdata/parity/cases/imgc-too-small/images/b.jpg differ diff --git a/internal/push/testdata/parity/cases/imgc-too-small/labels.csv b/internal/push/testdata/parity/cases/imgc-too-small/labels.csv new file mode 100644 index 00000000..aa98ba50 --- /dev/null +++ b/internal/push/testdata/parity/cases/imgc-too-small/labels.csv @@ -0,0 +1,3 @@ +image_id,label +a.jpg,cat +b.jpg,dog diff --git a/internal/push/testdata/parity/cases/imgc-zero-byte/images/a.jpg b/internal/push/testdata/parity/cases/imgc-zero-byte/images/a.jpg index 2c5a1e73..6520d390 100644 Binary files a/internal/push/testdata/parity/cases/imgc-zero-byte/images/a.jpg and b/internal/push/testdata/parity/cases/imgc-zero-byte/images/a.jpg differ diff --git a/internal/push/testdata/parity/goldens.json b/internal/push/testdata/parity/goldens.json index c67d17e7..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" @@ -95,7 +107,7 @@ }, "imgc-nonsquare-swapped": { "errors": [ - "ImageResolutionValidator: Images with incorrect resolution found: ['images/b.jpg: (8, 4) (expected: [4, 8])', 'images/a.jpg: (8, 4) (expected: [4, 8])']" + "ImageResolutionValidator: Images with incorrect resolution found: ['images/b.jpg: (64, 32) (expected: [32, 64])', 'images/a.jpg: (64, 32) (expected: [32, 64])']" ], "verdict": "reject" }, @@ -113,25 +125,29 @@ }, "imgc-res-mismatch": { "errors": [ - "ImageResolutionValidator: Multiple image resolutions found: [(4, 4), (8, 8)]. All images must have the same resolution.", - "ImageResolutionValidator: Expected resolution: [8, 8]", + "ImageResolutionValidator: Multiple image resolutions found: [(32, 32), (48, 48)]. All images must have the same resolution.", + "ImageResolutionValidator: Expected resolution: [32, 32]", "ImageResolutionValidator: Invalid files: []", - "ImageResolutionValidator: Resolution errors: ['images/odd.jpg: (4, 4) (expected: [8, 8])']" + "ImageResolutionValidator: Resolution errors: ['images/odd.jpg: (48, 48) (expected: [32, 32])']" ], "verdict": "reject" }, - "imgc-zero-byte": { + "imgc-too-small": { "errors": [ - "ImageResolutionValidator: Files that could not be processed: ['images/z.jpg: empty file (0 bytes)']" + "ImageResolutionValidator: Images below the minimum size (32, 32) (width, height) were found \u2014 they are too small to train on. Provide larger images or, if your model accepts smaller inputs, lower the floor via file_options.min_size. Offending files: ['images/b.jpg: (16, 16)', 'images/a.jpg: (16, 16)']" ], "verdict": "reject" }, - "tabular-bom": { + "imgc-zero-byte": { "errors": [ - "DataValidator: Schema columns not present in CSV: age." + "ImageResolutionValidator: Files that could not be processed: ['images/z.jpg: empty file (0 bytes)']" ], "verdict": "reject" }, + "tabular-bom": { + "errors": [], + "verdict": "accept" + }, "tabular-dup-header": { "errors": [ "DataValidator: Duplicate column name(s) in the CSV header: ['age']. Each column must be unique \u2014 otherwise the second is silently renamed '.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 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