From f54e06c4124eb3a31fdf3479fe6d2a59f43bba0a Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 9 Jul 2026 20:13:15 +0500 Subject: [PATCH] =?UTF-8?q?fix(data=20ingest):=20#180b=20review=20?= =?UTF-8?q?=E2=80=94=20path=20trim,=20self-supervised=20registry=20flag,?= =?UTF-8?q?=20honest=20text=20sniff=20echo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Trim the interactive path answer before storing it. validateDatasetPath only trims to check for emptiness, so a pasted " ~/data" (stray space) otherwise survived, defeated expandHome (first char isn't '~'), and made filepath.Abs prepend cwd — the family sniff / label-header preview then read a path that doesn't exist (and silently fell back to free-text label entry, the data-ingestors#340 class this feature exists to prevent). - Make SelfSupervised a CategorySpec registry field (set on MLM + CLM) and have SelfSupervisedText read it, so a new self-supervised task can't be added without deciding whether it needs a label column — was a hardcoded two-id switch decoupled from the registry. - Soften the confident text sniff echo to "looks like": texts/ and sequences/ map to DIFFERENT tasks (TextSidecarDir), so a family-level echo must not imply the task the user then picks will load. The walk stays the authoritative check. - Drop the unreachable empty-slice branch in defaultLabelChoice (its only caller already guards len(headers) > 0). Co-Authored-By: Claude Opus 4.8 --- internal/cli/interactive.go | 14 +++++++++----- internal/cli/interactive_test.go | 26 ++++++++++++++++++++++++++ internal/push/category.go | 19 +++++++++++++++---- internal/push/preview.go | 9 ++++++++- 4 files changed, 58 insertions(+), 10 deletions(-) diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index fe357947..5f28b277 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -141,7 +141,12 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo if err != nil { return err } - a.LocalPath = ans + // Trim before storing: validateDatasetPath only trims to check for + // emptiness, so a pasted " ~/data" (stray leading/trailing space) + // would otherwise survive here and defeat expandHome (first char + // isn't '~') — filepath.Abs then prepends cwd and the sniff / label + // preview read a path that doesn't exist. + a.LocalPath = strings.TrimSpace(ans) prompted = true } // Expand a leading ~ now so the family sniff + label-header preview read @@ -371,10 +376,9 @@ func defaultLabelChoice(headers []string) string { return h } } - if len(headers) > 0 { - return headers[0] - } - return "" + // The only caller (promptLabelColumn) guards len(headers) > 0 before + // calling, so headers is never empty here. + return headers[0] } // renderReview prints the assembled ingest inputs before the confirm diff --git a/internal/cli/interactive_test.go b/internal/cli/interactive_test.go index 3cdfe95c..529ed740 100644 --- a/internal/cli/interactive_test.go +++ b/internal/cli/interactive_test.go @@ -461,6 +461,32 @@ func TestRunInteractive_RejectsEmptyPath(t *testing.T) { } } +// TestRunInteractive_TrimsPath: a path answer with surrounding whitespace +// (a common paste artifact) is trimmed before it's stored, so expandHome +// and the family sniff read the real path rather than a cwd-prefixed +// mangle. Without the trim, " " defeats expandHome and the sniff +// would land in the wrong place. +func TestRunInteractive_TrimsPath(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "What should we call this dataset?": "t", + "Where is your data? (the folder holding it)": " " + dir + " ", + "Which column holds the class?": "churned", + }} + a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} + if err := runInteractive(discardPrinter(), f, a, false); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.LocalPath != dir { + t.Errorf("LocalPath = %q, want %q (surrounding whitespace not trimmed)", a.LocalPath, dir) + } + // The trimmed path must have sniffed cleanly as tabular (not landed in a + // cwd-prefixed nonexistent dir that would force the family question). + if a.Spec.Category != "tabular_classification" { + t.Errorf("Category = %q, want tabular_classification (sniff read the trimmed path)", a.Spec.Category) + } +} + // TestRunInteractive_ShowsExampleHints: the name and path prompts carry a // visible example, so the guided flow teaches as it goes. func TestRunInteractive_ShowsExampleHints(t *testing.T) { diff --git a/internal/push/category.go b/internal/push/category.go index e22aebc2..9edec1f8 100644 --- a/internal/push/category.go +++ b/internal/push/category.go @@ -35,6 +35,13 @@ type CategorySpec struct { // therefore need label.policy (object label form) so the raw target // never ships to the central backend by default. RegressionClass bool + // SelfSupervised marks text categories that train without an explicit + // label column — the target is derived from the text itself (MLM masks + // tokens; CLM predicts the next token), so the interactive flow skips + // the "which column is the label?" question. A registry fact rather + // than a hardcoded id list so a new self-supervised task can't be added + // without deciding this (SelfSupervisedText reads it). + SelfSupervised bool // CLISupported reports whether `dataset push` implements the category // today. semantic_segmentation is known (the schema defines it) but // not yet pushable. @@ -77,7 +84,7 @@ var categoryRegistry = []CategorySpec{ Blurb: "locate landmark points on an image (e.g. pose)"}, {ID: "text_classification", Family: FamilyText, Label: "Text classification", CLISupported: true, Blurb: "sort text snippets into classes"}, - {ID: "masked_language_modeling", Family: FamilyText, Label: "Masked language modeling", Gloss: "fill-mask", CLISupported: true, + {ID: "masked_language_modeling", Family: FamilyText, Label: "Masked language modeling", Gloss: "fill-mask", CLISupported: true, SelfSupervised: true, Blurb: "predict masked-out words — no labels needed"}, {ID: "tabular_classification", Family: FamilyTabular, Label: "Tabular classification", CLISupported: true, Blurb: "predict a class from table columns"}, @@ -90,7 +97,7 @@ var categoryRegistry = []CategorySpec{ {ID: "semantic_segmentation", Family: FamilyImage, Label: "Semantic segmentation", CLISupported: false, Blurb: "label every pixel in an image", UnsupportedNote: "blocked on the ingestor's mask-sidecar support (data-ingestors#136)"}, - {ID: "causal_language_modeling", Family: FamilyText, Label: "Causal language modeling", CLISupported: false, + {ID: "causal_language_modeling", Family: FamilyText, Label: "Causal language modeling", CLISupported: false, SelfSupervised: true, Blurb: "predict the next word in a sequence", UnsupportedNote: "schema-recognized (data-ingestors#805); `tracebloc ingest` discover/build for its raw-.txt / prompt\\tcompletion `texts` layout is pending"}, {ID: "seq2seq", Family: FamilyText, Label: "Sequence-to-sequence", Gloss: "translation / summarization", CLISupported: false, @@ -201,9 +208,13 @@ func FamilyNouns() []string { // SelfSupervisedText reports whether a text category trains without an // explicit label column — the target is derived from the text itself, so // the CLI skips the "which column is the label?" question. MLM masks -// tokens; CLM predicts the next token; neither reads a labels column. +// tokens; CLM predicts the next token; neither reads a labels column. The +// answer is the registry's SelfSupervised flag, so a new self-supervised +// task is handled the moment it's added to the registry — not when someone +// remembers to edit this function. func SelfSupervisedText(category string) bool { - return category == "masked_language_modeling" || category == "causal_language_modeling" + c, ok := categoryByID[category] + return ok && c.SelfSupervised } // IsKnown reports whether category is a recognized task category (in the diff --git a/internal/push/preview.go b/internal/push/preview.go index bf195e7e..47c05d1e 100644 --- a/internal/push/preview.go +++ b/internal/push/preview.go @@ -117,8 +117,15 @@ func SniffFamily(path string) FamilySniff { if hasSequences { dir = "sequences/" } + // "looks like", not "is": the family is confident, but texts/ and + // sequences/ map to DIFFERENT tasks (DiscoverText keys the sidecar + // dir off TextSidecarDir — texts/ for classification, sequences/ for + // MLM). So a confident text sniff must not imply the task the user + // then picks will load; the picker offers the whole text family, and + // the walk gives the authoritative error if the layout and the + // chosen task disagree. return FamilySniff{Family: FamilyText, Confident: true, - Echo: fmt.Sprintf("Found labels.csv and a %s folder — this is text data.", dir)} + Echo: fmt.Sprintf("Found labels.csv and a %s folder — this looks like text data.", dir)} case !hasImages && !hasText && csvCount == 1: // Exactly one CSV, mirroring DiscoverTabular's findSingleCSV rule. // Two or more CSVs is a directory the tabular walk rejects, so stay