diff --git a/internal/cli/data.go b/internal/cli/data.go index 31051444..c502cb9c 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -133,9 +133,9 @@ func newDataIngestCmd() *cobra.Command { contextOverride string nsOverride string - // Ingest-spec flags. image_classification + the tabular / - // time-series family are supported today; text + detection + - // segmentation land in later increments. + // Ingest-spec flags. All schema task categories are CLI-supported now + // (image classification / detection / segmentation / keypoint, the full + // text family, and the tabular / time-series family). // // --name/--task are the canonical flags (#180); --table/--category // stay on as hidden deprecated aliases so existing scripts keep @@ -612,12 +612,10 @@ collaborators can train against that table without ever seeing the raw files.`)) // 2. Category gate. Runs BEFORE schema validation so an // unsupported category gets a clear, actionable CLI message // rather than the schema's terse enum / missing-property error. - // Supported today: image_classification + the tabular / - // time-series family. The other image categories need sidecar - // (annotation/mask) staging the CLI doesn't do yet, and the - // text family needs a texts/sequences dir — both land in later - // increments. A typo'd category also lands here with a clear - // list rather than the schema's 11-option enum dump. + // Every schema task category is CLI-supported now; this gate stays as + // defensive routing so a future known-but-not-yet-wired category gets a + // clear per-category message, and a typo'd category gets the supported + // list rather than the schema's raw enum dump. switch { case a.Spec.Category == "": // No task chosen. In guided mode the picker already filled this; @@ -631,16 +629,19 @@ collaborators can train against that table without ever seeing the raw files.`)) case push.IsCLISupported(a.Spec.Category): // supported case push.IsKnown(a.Spec.Category): - // A recognized category data ingest doesn't implement yet — today just - // semantic_segmentation (awaiting the ingestor's mask_id link column + - // training sign-off, backend#816). Routed here (not the default branch) - // so the user gets the registry's per-category pending-support reason, + // A recognized category the CLI doesn't implement yet. None today — every + // schema category is wired — but kept as defensive routing so a future + // known-but-unsupported category gets the registry's per-category reason, // not a misleading "unrecognized category". Supported categories were // already caught above, so IsKnown here means known-but-unsupported. spec, _ := push.Lookup(a.Spec.Category) + reason := "" + if spec.UnsupportedNote != "" { + reason = " (" + spec.UnsupportedNote + ")" + } return &exitError{code: 2, err: fmt.Errorf( - "task %q isn't supported by the CLI yet (%s). Supported tasks: %s.", - a.Spec.Category, spec.UnsupportedNote, push.SupportedCategoriesList())} + "task %q isn't supported by the CLI yet%s. Supported tasks: %s.", + a.Spec.Category, reason, push.SupportedCategoriesList())} default: return &exitError{code: 2, err: fmt.Errorf( "task %q isn't a recognized task. Supported tasks: %s.", @@ -716,6 +717,8 @@ collaborators can train against that table without ever seeing the raw files.`)) layout, err = push.DiscoverText(a.Spec.Category, a.LocalPath) case a.Spec.Category == "object_detection": layout, err = push.DiscoverObjectDetection(a.LocalPath) + case a.Spec.Category == "semantic_segmentation": + layout, err = push.DiscoverSemanticSegmentation(a.LocalPath) default: // image_classification + keypoint_detection: labels.csv + images/. layout, err = push.Discover(a.LocalPath) @@ -859,16 +862,18 @@ collaborators can train against that table without ever seeing the raw files.`)) // the registry's SelfSupervised flag (not a hardcoded id). } - // 3b. Friendly missing-label pre-check (#214). Every tabular / time-series - // task carries a label column (layout contract has_label_column=true for - // the whole family). With no --label-column the synthesized spec's - // `label` is an empty string, which trips the schema's label oneOf and - // the raw validation below dumps an opaque "got object, want string" / - // "minLength" pair. Intercept ONLY that specific missing case here — a - // label that's present-but-not-in-the-CSV still flows to - // runLocalPreflight's CheckLabelColumn, and every other schema error - // still reaches the dump — and name the flag to fix instead. - if push.IsTabular(a.Spec.Category) && a.Spec.LabelColumn == "" { + // 3b. Friendly missing-label pre-check (#214). Tabular / time-series tasks + // AND semantic_segmentation carry a required label column (the ingest + // schema's allOf requires `label` for them). With no --label-column the + // synthesized spec's `label` is an empty string, which trips the schema's + // label oneOf and the raw validation below dumps an opaque "got object, + // want string" / "minLength" pair. semseg is especially prone to this — + // its per-image label reads as vestigial beside the pixel masks, so the + // flag is easy to forget. Intercept ONLY that specific missing case here — + // a label present-but-not-in-the-CSV still flows to runLocalPreflight's + // CheckLabelColumn, and every other schema error still reaches the dump — + // and name the flag to fix instead. + if (push.IsTabular(a.Spec.Category) || a.Spec.Category == "semantic_segmentation") && a.Spec.LabelColumn == "" { msg := "this task needs a label column, but --label-column wasn't set — " + "pass --label-column with the name of the target column in your data CSV" if cols := sortedKeys(a.Spec.Schema); len(cols) > 0 { @@ -1318,6 +1323,9 @@ func printLocalSummary(p *ui.Printer, layout *push.LocalLayout, spec map[string] if anns := layout.Sidecars["annotations"]; len(anns) > 0 { p.Field("annotations", fmt.Sprintf("%d files", len(anns))) } + if masks := layout.Sidecars["masks"]; len(masks) > 0 { + p.Field("masks", fmt.Sprintf("%d files", len(masks))) + } } p.Field("total size", push.HumanBytes(layout.TotalBytes)) diff --git a/internal/cli/data_test.go b/internal/cli/data_test.go index ba82c7e1..79caf995 100644 --- a/internal/cli/data_test.go +++ b/internal/cli/data_test.go @@ -87,15 +87,13 @@ func execDataIngest(t *testing.T, args []string) (exitCode int, stdout, stderr s // TestDataIngest_UnsupportedCategory_ExitsTwo: the CLI-side category // gate runs before schema validation so a customer who passes a // not-yet-supported category gets an actionable message (exit 2) -// rather than the schema's confusing missing-property error. Today's -// supported set is image_classification + the tabular / time-series -// family; the other image categories (which need annotation/mask -// sidecar staging), the text family, and nonsense values are gated -// out here. Bugbot review-on-self caught the missing gate on PR-a. +// rather than the schema's confusing missing-property error. Every schema +// category is CLI-supported now, so only a dead/removed category +// (instance_segmentation) or a nonsense value is gated out here. Bugbot +// review-on-self caught the missing gate on PR-a. func TestDataIngest_UnsupportedCategory_ExitsTwo(t *testing.T) { root := imgcLayout(t) for _, badCategory := range []string{ - "semantic_segmentation", // known but pending (awaiting mask_id + training sign-off, backend#816) "instance_segmentation", // dead — removed from the registry (#1005), now unrecognized "definitely-not-a-category", // nonsense; gate catches this too } { @@ -114,36 +112,11 @@ func TestDataIngest_UnsupportedCategory_ExitsTwo(t *testing.T) { } } -// TestDataIngest_KnownUnsupportedCategory_PendingNote pins the Bugbot fix -// (v0.4.0 RC): a registry-known but CLI-unsupported category -// (semantic_segmentation — the sole remaining one after phase 4) must get the -// registry's pending-support note, not the misleading "isn't a recognized task -// category" message. execDataIngest discards the error and SilenceErrors -// swallows it, so run the command here and inspect the returned error directly. -func TestDataIngest_KnownUnsupportedCategory_PendingNote(t *testing.T) { - root := imgcLayout(t) - rootCmd := NewRootCmd(BuildInfo{Version: "test"}) - rootCmd.SetOut(&bytes.Buffer{}) - rootCmd.SetErr(&bytes.Buffer{}) - rootCmd.SetArgs([]string{"data", "ingest", - "--kubeconfig=/tmp/tracebloc-cli-test-nonexistent-" + t.Name(), - root, "--name=t1", "--task=semantic_segmentation", - "--intent=train", "--label-column=label"}) - err := rootCmd.Execute() - if err == nil { - t.Fatal("expected an error for a known-but-unsupported task") - } - if got := ExitCodeFromError(err); got != 2 { - t.Fatalf("exit code = %d, want 2", got) - } - msg := err.Error() - if strings.Contains(msg, "isn't a recognized task") { - t.Errorf("known task misrouted to the unrecognized-task branch:\n%s", msg) - } - if !strings.Contains(msg, "isn't supported by the CLI yet") { - t.Errorf("want the registry pending-support note, got:\n%s", msg) - } -} +// (Removed) TestDataIngest_KnownUnsupportedCategory_PendingNote pinned the +// pending-support routing for a known-but-CLI-unsupported category. Every schema +// category is wired now (#182 closed semantic_segmentation), so there is no such +// category to exercise it; the defensive IsKnown branch in data.go stays for a +// future one. // TestDataIngest_TraversalTableName_ExitsTwo is the security // regression pin at the CLI layer. --name=../../etc must be diff --git a/internal/cli/interactive_test.go b/internal/cli/interactive_test.go index c5e673bb..040d7203 100644 --- a/internal/cli/interactive_test.go +++ b/internal/cli/interactive_test.go @@ -335,10 +335,10 @@ func TestPickTask_FamilyScoped(t *testing.T) { } } -// TestPickTask_ImagePending: semantic_segmentation is the sole remaining -// CLI-pending task, so the image picker still renders a greyed "Not yet in the -// CLI" section with its backend#816 reason. -func TestPickTask_ImagePending(t *testing.T) { +// TestPickTask_ImageAllAvailable: after #182 wired semantic_segmentation, every +// image task is available in the CLI, so the image picker lists them all under +// "Available now:" with no greyed "Not yet in the CLI" pending section. +func TestPickTask_ImageAllAvailable(t *testing.T) { f := &fakePrompter{answers: map[string]string{"Which task?": "Image classification"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) @@ -349,14 +349,18 @@ func TestPickTask_ImagePending(t *testing.T) { for _, want := range []string{ "Available now:", "Image classification", - "Not yet in the CLI:", - "semantic_segmentation", - "backend#816", // the UnsupportedNote reason + "Semantic segmentation", // now selectable, no longer pending } { if !strings.Contains(out, want) { t.Errorf("image picker missing %q:\n%s", want, out) } } + // No pending section and no stale backend#816 note now that semseg is wired. + for _, unwanted := range []string{"Not yet in the CLI", "backend#816"} { + if strings.Contains(out, unwanted) { + t.Errorf("image picker still shows pending content %q:\n%s", unwanted, out) + } + } } // TestPickTask_TabularGloss: the tabular picker shows the survival-analysis diff --git a/internal/push/category.go b/internal/push/category.go index bbf2011d..7a8df80f 100644 --- a/internal/push/category.go +++ b/internal/push/category.go @@ -132,13 +132,14 @@ var categoryRegistry = []CategorySpec{ Blurb: "label how two texts relate"}, {ID: "embeddings", Family: FamilyText, Label: "Embeddings", CLISupported: true, SelfSupervised: true, Blurb: "learn vector representations from text pairs"}, - // semantic_segmentation stays CLI-pending: di#136 (mask sidecar) shipped, - // but the ingestor doesn't yet populate the mask_id link column the - // contract requires, and the training-side sign-off is tracked in - // backend#816. Wire it once those land (RFC-0002 phase 4 follow-up). - {ID: "semantic_segmentation", Family: FamilyImage, Label: "Semantic segmentation", CLISupported: false, IsClassification: true, - Blurb: "label every pixel in an image", - UnsupportedNote: "schema-recognized; awaiting the ingestor's mask_id link column + training sign-off (backend#816)"}, + // semantic_segmentation: images/ + one PNG mask per image in masks/, linked + // by the manifest's mask_id column (backend#816 contract). Wired in RFC-0002 + // phase 4 (#182) now that its blockers landed — di#358 shipped the ingestor's + // require-and-enforce mask_id validator (ingestor v0.7.0) and backend#816 + // closed. The CLI stages the masks/ sidecar, declares mask_id in the schema, + // and previews the images↔masks (_mask-suffix) pairing + the mask_id contract. + {ID: "semantic_segmentation", Family: FamilyImage, Label: "Semantic segmentation", CLISupported: true, IsClassification: true, + Blurb: "label every pixel in an image"}, } // categoryByID indexes the registry for O(1) lookup, built once. diff --git a/internal/push/category_registry_test.go b/internal/push/category_registry_test.go index 40975ad3..67bc29c5 100644 --- a/internal/push/category_registry_test.go +++ b/internal/push/category_registry_test.go @@ -38,42 +38,33 @@ func TestRegistryKnownCategories(t *testing.T) { func TestSupportedCategories(t *testing.T) { got := SupportedCategoryIDs() - // RFC-0002 phase 4 wired the 5 text tasks (token/sentence-pair - // classification, causal LM, seq2seq, embeddings) and backend#1054 WS2 - // added time_series_classification, so 15 of the 16 categories are - // pushable; only semantic_segmentation remains pending. - if len(got) != 15 { - t.Fatalf("SupportedCategoryIDs() len = %d, want 15: %v", len(got), got) + // RFC-0002 phase 4 wired the last pending category — semantic_segmentation + // (#182; its blockers landed — di#358 shipped the ingestor's mask_id + // require-and-enforce in v0.7.0, and backend#816 closed). So ALL 16 schema + // categories are pushable now, none gated out. + if len(got) != 16 { + t.Fatalf("SupportedCategoryIDs() len = %d, want 16: %v", len(got), got) } for _, id := range got { if !IsCLISupported(id) { t.Errorf("SupportedCategoryIDs returned %q but IsCLISupported is false", id) } } - // semantic_segmentation is the sole known-but-not-yet-pushable category - // (awaiting the ingestor's mask_id link column + training sign-off, - // backend#816); it must stay gated out and explain why. - for _, id := range []string{"semantic_segmentation"} { - if !IsKnown(id) { - t.Errorf("%s should be known", id) - } - if IsCLISupported(id) { - t.Errorf("%s should not be CLI-supported yet", id) - } - if spec, _ := Lookup(id); spec.UnsupportedNote == "" { - t.Errorf("%s should carry an UnsupportedNote", id) - } - } - // The 5 newly-wired text tasks must now be pushable AND carry no stale - // pending note (the picker only greys out categories with a note). - for _, id := range []string{"token_classification", "sentence_pair_classification", "causal_language_modeling", "seq2seq", "embeddings"} { + // Every known category is now CLI-supported AND carries no stale pending + // note (the picker only greys out categories with an UnsupportedNote). + for _, id := range allCategoryIDs() { if !IsCLISupported(id) { - t.Errorf("%s should be CLI-supported after phase 4", id) + t.Errorf("%s should be CLI-supported (every category is wired now)", id) } if spec, _ := Lookup(id); spec.UnsupportedNote != "" { t.Errorf("%s is supported but still carries an UnsupportedNote: %q", id, spec.UnsupportedNote) } } + // semantic_segmentation specifically — the phase-4 finale (#182) — must be + // pushable now, closing out the known-but-pending set. + if !IsCLISupported("semantic_segmentation") { + t.Error("semantic_segmentation should be CLI-supported after #182") + } } func TestPredicatesDeriveFromRegistry(t *testing.T) { diff --git a/internal/push/image_extras.go b/internal/push/image_extras.go index 5e1e0f15..8db0ebbd 100644 --- a/internal/push/image_extras.go +++ b/internal/push/image_extras.go @@ -9,6 +9,12 @@ import ( // (Pascal VOC XML). var xmlExtensions = map[string]struct{}{".xml": {}} +// pngExtensions are the mask file types semantic_segmentation reads. The +// ingestor's FileTypeValidator forces masks to .png (modalities/validators.py +// semantic_segmentation) and the layout contract's masks sidecar globs *.png, +// so the CLI mirrors that here. +var pngExtensions = map[string]struct{}{".png": {}} + // DiscoverObjectDetection validates a local object_detection dataset: // // - /labels.csv (required) @@ -49,3 +55,47 @@ func DiscoverObjectDetection(rootDir string) (*LocalLayout, error) { } return layout, nil } + +// DiscoverSemanticSegmentation validates a local semantic_segmentation dataset: +// +// - /labels.csv (required; must declare + populate a mask_id column) +// - /images/* (required) +// - /masks/*.png (required; one PNG mask per image) +// +// Like object_detection it builds on the image-classification layout +// (labels.csv + images/) and adds a sidecar — here masks/ — via the shared +// sidecar walker, so the existing tar/stream machinery stages masks under +// "masks/". The images↔masks pairing (by the `_mask` filename suffix) and the +// mask_id link-column contract are previewed in preflight (CheckMaskPairing, +// CheckMaskIdColumn), mirroring the ingestor's FilePairingValidator + +// MaskIdColumnValidator (modalities/validators.py, backend#816). +func DiscoverSemanticSegmentation(rootDir string) (*LocalLayout, error) { + layout, err := Discover(rootDir) // labels.csv + images/ (+ caps + symlink guards) + if err != nil { + return nil, err + } + + masks, maskBytes, err := discoverSidecarFiles(layout.Root, "masks", pngExtensions) + if err != nil { + return nil, err + } + if len(masks) == 0 { + return nil, fmt.Errorf( + "no .png mask files found in %q. semantic_segmentation expects "+ + "/masks/*.png (one PNG mask per image, named _mask.png).", + filepath.Join(layout.Root, "masks")) + } + if layout.Sidecars == nil { + layout.Sidecars = map[string][]string{} + } + layout.Sidecars["masks"] = masks + layout.TotalBytes += maskBytes + + if layout.TotalBytes > MaxTotalBytes { + return nil, fmt.Errorf( + "dataset is %s, exceeds v0.1 cap of %s. For larger datasets, the "+ + "cloud-source path is on the v0.2 roadmap (tracebloc/client#147).", + HumanBytes(layout.TotalBytes), HumanBytes(MaxTotalBytes)) + } + return layout, nil +} diff --git a/internal/push/image_extras_test.go b/internal/push/image_extras_test.go index 8ed8d718..d5f749f3 100644 --- a/internal/push/image_extras_test.go +++ b/internal/push/image_extras_test.go @@ -57,3 +57,56 @@ func TestDiscoverObjectDetection_MissingAnnotations(t *testing.T) { t.Error("DiscoverObjectDetection without annotations/ returned nil error") } } + +// mkSemsegDir builds a semantic_segmentation dataset dir: labels.csv (with the +// required mask_id column) + images/001.jpg, plus masks/001_mask.png when +// withMasks (the shipped _mask.png convention, #196). +func mkSemsegDir(t *testing.T, withMasks bool) string { + t.Helper() + dir := t.TempDir() + writeFile(t, dir, "labels.csv", "image_label,filename,mask_id\ncat,001.jpg,001_mask\n") + imgs := filepath.Join(dir, "images") + if err := os.MkdirAll(imgs, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(imgs, "001.jpg"), []byte("\xff\xd8\xff\xe0"), 0o644); err != nil { + t.Fatal(err) + } + if withMasks { + masks := filepath.Join(dir, "masks") + if err := os.MkdirAll(masks, 0o755); err != nil { + t.Fatal(err) + } + // PNG magic bytes; discoverSidecarFiles keys on the extension, not decode. + if err := os.WriteFile(filepath.Join(masks, "001_mask.png"), []byte("\x89PNG\r\n\x1a\n"), 0o644); err != nil { + t.Fatal(err) + } + } + return dir +} + +// TestDiscoverSemanticSegmentation: a valid semseg layout yields images + the +// masks sidecar, staged together. +func TestDiscoverSemanticSegmentation(t *testing.T) { + layout, err := DiscoverSemanticSegmentation(mkSemsegDir(t, true)) + if err != nil { + t.Fatalf("DiscoverSemanticSegmentation: %v", err) + } + if len(layout.Images) != 1 { + t.Errorf("images = %d, want 1", len(layout.Images)) + } + if len(layout.Sidecars["masks"]) != 1 { + t.Errorf("masks = %d, want 1", len(layout.Sidecars["masks"])) + } + if got := layout.FileCount(); got != 3 { // labels.csv + image + mask + t.Errorf("FileCount = %d, want 3", got) + } +} + +// TestDiscoverSemanticSegmentation_MissingMasks: semseg without a masks/ +// directory is a clear error. +func TestDiscoverSemanticSegmentation_MissingMasks(t *testing.T) { + if _, err := DiscoverSemanticSegmentation(mkSemsegDir(t, false)); err == nil { + t.Error("DiscoverSemanticSegmentation without masks/ returned nil error") + } +} diff --git a/internal/push/layout_contract_test.go b/internal/push/layout_contract_test.go index ccc7a755..fdfb5c81 100644 --- a/internal/push/layout_contract_test.go +++ b/internal/push/layout_contract_test.go @@ -63,6 +63,35 @@ func TestRegistryMirrorsLayoutContract(t *testing.T) { // contract's primary_subdir for every text task — the directory the CLI stages // into has to be the one the ingestor reads (texts/ for every text task but // MLM, which uses sequences/). +// TestSemsegSidecarMirrorsContract pins the semseg sidecar facts the Go code +// hardcodes — "masks"/pngExtensions (image_extras.go), the mask_id link column +// (preflight.go CheckMaskIdColumn), and spec["schema"]={mask_id} (spec.go) — to +// the vendored layout contract, which the ingestor owns (RFC-0002 Principle 6). +// If the ingestor renames the link column or changes the mask subdir/glob, this +// fails rather than the CLI silently emitting and checking the stale name. +func TestSemsegSidecarMirrorsContract(t *testing.T) { + layout, ok := LayoutFor("semantic_segmentation") + if !ok { + t.Fatal("semantic_segmentation missing from layout.v1.json") + } + if len(layout.Sidecars) != 1 { + t.Fatalf("semseg sidecars = %d, want 1", len(layout.Sidecars)) + } + sc := layout.Sidecars[0] + if sc.Subdir != "masks" { + t.Errorf(`sidecar subdir = %q, want "masks" (hardcoded in image_extras.go/spec.go)`, sc.Subdir) + } + if sc.Glob != "*.png" { + t.Errorf(`sidecar glob = %q, want "*.png" (pngExtensions in image_extras.go)`, sc.Glob) + } + if sc.LinkColumn == nil || *sc.LinkColumn != "mask_id" { + t.Errorf(`sidecar link_column = %v, want "mask_id" (maskIDColumn in preflight.go, schema key in spec.go)`, sc.LinkColumn) + } + if !sc.Required { + t.Error("semseg masks sidecar should be required=true") + } +} + func TestTextSidecarDirMirrorsContract(t *testing.T) { for _, c := range categoryRegistry { if c.Family != FamilyText { diff --git a/internal/push/preflight.go b/internal/push/preflight.go index 0cb237df..950c944e 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -436,6 +436,163 @@ func CheckAnnotationPairing(images, annotations []string) error { strings.Join(parts, "; ")) } +// CheckMaskPairing previews the ingestor's FilePairingValidator for +// semantic_segmentation (modalities/validators.py, sidecar_suffix="_mask"): +// every image must have a PNG mask named "_mask.png" in masks/ and +// vice versa. The ingestor strips the documented "_mask" suffix (#196) from +// mask stems before matching, so image_001.jpg pairs with image_001_mask.png; +// a mask NOT carrying the suffix can't pair and is a non-conforming orphan. A +// mismatch in any direction fails in-cluster after the upload. +func CheckMaskPairing(images, masks []string) error { + const maxListed = 5 + imgStems := make(map[string]bool, len(images)) + for _, p := range images { + base := filepath.Base(p) + if strings.HasPrefix(base, ".") { + continue // hidden file (e.g. macOS AppleDouble ._x) — FilePairingValidator._stems skips these + } + imgStems[strings.TrimSuffix(base, filepath.Ext(base))] = true + } + // Strip the extension then the "_mask" suffix so a mask maps back to the + // image stem it pairs with; a mask stem without the suffix is kept as a + // non-conforming orphan (mirrors the validator's extra_orphans). + maskFor := make(map[string]bool, len(masks)) + var nonConforming []string + for _, p := range masks { + base := filepath.Base(p) + if strings.HasPrefix(base, ".") { + continue // hidden file — mirror the ingestor's _stems, so a stray ._x.png can't fake a mismatch + } + stem := strings.TrimSuffix(base, filepath.Ext(base)) + if strings.HasSuffix(stem, "_mask") { + maskFor[strings.TrimSuffix(stem, "_mask")] = true + } else { + nonConforming = append(nonConforming, base) + } + } + var noMask, noImg []string + for s := range imgStems { + if !maskFor[s] { + noMask = append(noMask, s) + } + } + for s := range maskFor { + if !imgStems[s] { + noImg = append(noImg, s+"_mask") + } + } + if len(noMask) == 0 && len(noImg) == 0 && len(nonConforming) == 0 { + return nil + } + sort.Strings(noMask) + sort.Strings(noImg) + sort.Strings(nonConforming) + var parts []string + if len(noMask) > 0 { + parts = append(parts, fmt.Sprintf("%d image(s) without a mask (%s)", + len(noMask), TruncateList(noMask, maxListed))) + } + if len(noImg) > 0 { + parts = append(parts, fmt.Sprintf("%d mask(s) without an image (%s)", + len(noImg), TruncateList(noImg, maxListed))) + } + if len(nonConforming) > 0 { + parts = append(parts, fmt.Sprintf("%d mask(s) not named _mask.png (%s)", + len(nonConforming), TruncateList(nonConforming, maxListed))) + } + return fmt.Errorf( + "images/ and masks/ don't pair up: %s. Every image needs a same-named "+ + "\"_mask.png\" in masks/ (and vice versa) — the cluster rejects mismatches "+ + "after the upload.", + strings.Join(parts, "; ")) +} + +// CheckMaskIdColumn previews the ingestor's MaskIdColumnValidator +// (validators/mask_id_validator.py, backend#816) for semantic_segmentation: the +// manifest must DECLARE a mask_id column AND POPULATE it on every row. The +// training client resolves each mask file from this column with no naming- +// convention fallback, so an undeclared or empty mask_id becomes a late, opaque +// FileNotFoundError at train time. The name is the exact lowercase "mask_id" +// (the stored table keys on it verbatim; a different-case column stores nothing +// the client can read), so a case variant gets a rename hint, not a silent pass. +func CheckMaskIdColumn(csvPath string) error { + const maskIDColumn = "mask_id" + header, err := ReadCSVHeader(csvPath) + if err != nil { + return fmt.Errorf("reading %s: %w", filepath.Base(csvPath), err) + } + exact := -1 + for i, c := range header { + if c == maskIDColumn { + exact = i + break + } + } + if exact == -1 { + // Distinguish a wrong-case/whitespace variant (rename) from truly absent + // (add), mirroring the ingestor's undeclared-vs-miscased split. + if v := matchColumnIndex(header, maskIDColumn); v >= 0 { + return fmt.Errorf( + "semantic_segmentation needs a %q column in %s, but found %q (wrong case). "+ + "Rename it to exactly %q — the training client reads that column to locate each mask.", + maskIDColumn, filepath.Base(csvPath), header[v], maskIDColumn) + } + return fmt.Errorf( + "semantic_segmentation needs a %q column in %s (columns: %s) linking each image to its "+ + "mask file. Add it — the training client reads it to locate each mask.", + maskIDColumn, filepath.Base(csvPath), strings.Join(header, ", ")) + } + // Populated on every row? Mirror the ingestor's NA-sentinel-aware empty scan + // (a missing cell counts as empty), keeping a bounded sample of row numbers. + r, closer, err := openCSVReader(csvPath) + if err != nil { + return fmt.Errorf("reading %s: %w", filepath.Base(csvPath), err) + } + defer func() { _ = closer.Close() }() + if _, err := r.Read(); err != nil { + return nil // no header/rows — CheckHasDataRows owns that diagnostic + } + const maxSample = 5 + var emptyRows []string + emptyCount, row := 0, 0 + for { + rec, rerr := r.Read() + if errors.Is(rerr, io.EOF) { + break + } + if rerr != nil { + // Fail closed on a mid-read error rather than pass a partial scan + // (matches CrossCheckLabels / #221). + return fmt.Errorf("reading %s: %w", filepath.Base(csvPath), rerr) + } + row++ + // Mirror the ingestor's _is_empty EXACTLY: a cell is empty iff its RAW + // (untrimmed) value is an NA-sentinel token, OR it's whitespace-only / + // missing. Trimming BEFORE the sentinel test would treat a PADDED + // sentinel like " NULL " as empty — but pandas keeps the spaces + // (skipinitialspace=False), so it's a REAL value in-cluster. That + // over-rejection is the cli#218/#239 padded-NA parity trap. + raw := "" + if exact < len(rec) { + raw = rec[exact] + } + if _, isNA := naSentinels[raw]; isNA || strings.TrimSpace(raw) == "" { + emptyCount++ + if len(emptyRows) < maxSample { + emptyRows = append(emptyRows, fmt.Sprintf("data row %d", row)) + } + } + } + if emptyCount > 0 { + return fmt.Errorf( + "%d row(s) in %s have an empty %q (e.g. %s). Every row must name its mask file — an "+ + "empty value makes the training client derive a garbage filename and fail. Fill in "+ + "%q or drop those rows, then re-run.", + emptyCount, filepath.Base(csvPath), maskIDColumn, strings.Join(emptyRows, ", "), maskIDColumn) + } + return nil +} + // TruncateList joins up to max items, appending "… and N more" past that. func TruncateList(items []string, max int) string { if len(items) <= max { @@ -1402,6 +1559,36 @@ func PreflightDataset(spec SpecArgs, layout *LocalLayout) (notes []string, probl if err := CheckAnnotationPairing(layout.Images, layout.Sidecars["annotations"]); err != nil { return nil, dataProblem(err) } + case "semantic_segmentation": + // images↔masks "_mask"-suffix pairing (FilePairingValidator preview) + // + the mask_id link-column contract (MaskIdColumnValidator preview, + // backend#816) + the labels.csv rows↔images/ existence check (same + // file_transfer failed-record preview image_classification runs). We + // deliberately do NOT run CheckLabelColumn: semseg requires a `label` + // field, but the ingestor omits LabelColumnValidator for it (labels + // come from the masks), so verifying the column is in the CSV would + // over-reject a dataset the cluster accepts. + if err := CheckMaskPairing(layout.Images, layout.Sidecars["masks"]); err != nil { + return nil, dataProblem(err) + } + if err := CheckMaskIdColumn(layout.LabelsCSV); err != nil { + return nil, dataProblem(err) + } + missing, orphanFiles, cerr := CrossCheckLabels(layout.LabelsCSV, layout.Images, spec.Extension) + if cerr != nil { + return nil, dataProblem(cerr) + } + if len(missing) > 0 { + return nil, dataProblem(fmt.Errorf( + "%d labels.csv row(s) reference images that aren't in images/: %s. Those records "+ + "would fail after the upload — fix the rows or add the files, then re-run.", + len(missing), TruncateList(missing, 5))) + } + if len(orphanFiles) > 0 { + notes = append(notes, fmt.Sprintf( + "Note: %d file(s) in images/ have no labels.csv row and won't be part of the dataset: %s", + len(orphanFiles), TruncateList(orphanFiles, 5))) + } } default: diff --git a/internal/push/preflight_test.go b/internal/push/preflight_test.go index 169de350..b01fff50 100644 --- a/internal/push/preflight_test.go +++ b/internal/push/preflight_test.go @@ -828,3 +828,81 @@ func TestOpenCSVReader_LazyQuotesMatchesPandas(t *testing.T) { v.Found, v.RowCount, v.Classes) } } + +// TestCheckMaskPairing mirrors the ingestor's FilePairingValidator for +// semantic_segmentation (sidecar_suffix="_mask"): images↔masks pair by +// _mask.png; a gap in either direction, or a mask not carrying the +// suffix, is reported. +func TestCheckMaskPairing(t *testing.T) { + cases := []struct { + name string + images []string + masks []string + wantErr string // substring; "" = no error + }{ + {"paired", []string{"images/001.jpg", "images/002.jpg"}, + []string{"masks/001_mask.png", "masks/002_mask.png"}, ""}, + {"image without mask", []string{"images/001.jpg", "images/002.jpg"}, + []string{"masks/001_mask.png"}, "without a mask"}, + {"mask without image", []string{"images/001.jpg"}, + []string{"masks/001_mask.png", "masks/002_mask.png"}, "without an image"}, + {"non-conforming mask name (no _mask suffix)", []string{"images/001.jpg"}, + []string{"masks/001.png"}, "not named _mask.png"}, + // A hidden file (macOS AppleDouble ._x) must be ignored, mirroring the + // ingestor's _stems — else a stray ._stray.png fakes a mismatch and + // over-rejects a dataset the cluster accepts. Fails without the dotfile skip. + {"hidden file with no counterpart is ignored", []string{"images/001.jpg"}, + []string{"masks/001_mask.png", "masks/._stray.png"}, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := CheckMaskPairing(tc.images, tc.masks) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("want nil, got %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("want error containing %q, got %v", tc.wantErr, err) + } + }) + } +} + +// TestCheckMaskIdColumn mirrors the ingestor's MaskIdColumnValidator +// (backend#816): the manifest must declare an exact-lowercase mask_id column and +// populate it on every row (NA-sentinel-aware); a wrong-case column gets a +// rename hint, an absent one an add hint. +func TestCheckMaskIdColumn(t *testing.T) { + cases := []struct { + name string + csv string + wantErr string + }{ + {"valid", "image_label,filename,mask_id\ncat,001.jpg,001_mask\n", ""}, + {"missing column", "image_label,filename\ncat,001.jpg\n", `needs a "mask_id" column`}, + {"wrong case", "filename,Mask_Id\n001.jpg,001_mask\n", "wrong case"}, + {"empty value on a row", "filename,mask_id\n001.jpg,001_mask\n002.jpg,\n", "empty"}, + {"NA-sentinel value", "filename,mask_id\n001.jpg,NULL\n", "empty"}, + // A PADDED NA token is a REAL value in-cluster (pandas keeps the spaces), + // so the CLI must not flag it empty — the cli#218/#239 parity trap. Fails + // if the scan trims before the naSentinels test. + {"padded NA token is a real value, not empty", "filename,mask_id\n001.jpg, NULL \n", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := writeTmp(t, "labels.csv", []byte(tc.csv)) + err := CheckMaskIdColumn(p) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("want nil, got %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("want error containing %q, got %v", tc.wantErr, err) + } + }) + } +} diff --git a/internal/push/spec.go b/internal/push/spec.go index 6d7f3f6d..e4df1d1b 100644 --- a/internal/push/spec.go +++ b/internal/push/spec.go @@ -309,6 +309,15 @@ func (a SpecArgs) buildImage(spec map[string]any, prefix string) { if a.Category == "object_detection" { spec["annotations"] = path.Join(prefix, "annotations") + "/" } + if a.Category == "semantic_segmentation" { + spec["masks"] = path.Join(prefix, "masks") + "/" + // Declare the mask_id link column so the ingestor STORES it: an + // undeclared mask_id is silently dropped and the training client then + // can't locate masks (backend#816). VARCHAR(255) + this top-level + // schema key match the canonical example spec + // (data-ingestors/examples/yaml/semantic_segmentation.yaml). + spec["schema"] = map[string]string{"mask_id": "VARCHAR(255)"} + } // file_options carries the per-file conventions the ingestor's // validators read: the detected extension (all categories in the diff --git a/internal/push/spec_test.go b/internal/push/spec_test.go index bf2267e4..2e131bae 100644 --- a/internal/push/spec_test.go +++ b/internal/push/spec_test.go @@ -181,6 +181,47 @@ func TestBuild_WithTargetSize_PassesSchema(t *testing.T) { } } +// TestBuild_SemanticSegmentation pins the semseg spec emission (#182). The +// mask_id schema declaration is the backend#816 crux — an undeclared mask_id is +// dropped and the training client then can't locate masks — so a mutation that +// drops spec["schema"], renames spec["masks"], or changes the SQL type must fail +// here rather than silently ship. The spec must also validate against the schema. +func TestBuild_SemanticSegmentation(t *testing.T) { + spec := SpecArgs{ + Table: "tumors_train", + Category: "semantic_segmentation", + Intent: "train", + LabelColumn: "image_label", + Extension: ".jpg", + TargetSize: []int{256, 256}, + }.Build() + + if s, _ := spec["masks"].(string); !strings.HasSuffix(s, "masks/") { + t.Errorf(`spec["masks"] = %#v, want a path ending in "masks/" (Build prepends the staging prefix)`, spec["masks"]) + } + sch, ok := spec["schema"].(map[string]string) + if !ok || sch["mask_id"] != "VARCHAR(255)" { + t.Fatalf(`spec["schema"] = %#v, want {mask_id: VARCHAR(255)} (backend#816 declaration)`, spec["schema"]) + } + + 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("semseg spec failed schema validation: %s\nspec:\n%s", + schema.FormatErrors(errs), specBytes) + } +} + // 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