diff --git a/internal/cli/copy_catalog_test.go b/internal/cli/copy_catalog_test.go index c4d5959..5826d4e 100644 --- a/internal/cli/copy_catalog_test.go +++ b/internal/cli/copy_catalog_test.go @@ -131,7 +131,7 @@ func TestCopyCatalog(t *testing.T) { p.Hintf("For help: https://docs.tracebloc.io/create-use-case/prepare-dataset") pr := &catalogPrompter{w: &b, answers: answers} a := &runDataIngestArgs{} - if err := runInteractive(p, pr, a, false /*taskSet*/); err != nil { + if err := runInteractive(p, pr, a); err != nil { t.Fatalf("driveIngest(%s): %v", dir, err) } return strings.ReplaceAll(b.String(), dir, shownPath) @@ -211,7 +211,7 @@ func TestCopyCatalog(t *testing.T) { } dataIngestFile := doc( "tb data ingest — stage a dataset into your secure environment", - "What you see when you run `tb data ingest` with no flags: a short intro, a\nfour-step guided setup (intent, name, path, task) then the task-specific\nquestions, and — after you confirm — the run itself. The setup is\ndriven through the real flow for one task in each family (tabular, image, text)\nso the task-specific questions are visible; each core question prints as a\n`Step N of 4 · …` header, the task-specific ones (the label column, and extras\nlike resolution or schema) as their own header, the\nsupporting line beneath it, and the `?` line shows your answer. The run (shown\nonce, for tabular) is the three steps + the final summary as the CLI renders\nthem. Passing flags (--as, --task, a path, …) skips the matching questions. The\nother tasks' extra questions (keypoints, label policy, time column),\nself-supervised text (which skips the label question), and the failure-summary\nwordings are in zz-all-strings.golden. The raw ingestor stream the CLI streams\nthrough (MySQL waits, the 📊 banner, per-validator lines) is the engine's own\nstdout — not CLI copy — so it isn't shown. (`tb ingest` is a hidden deprecated\nalias; `push` is a deprecated alias of the verb.)", + "What you see when you run `tb data ingest` with no flags: a short intro, a\nfour-step guided setup (intent, name, path, task) then the task-specific\nquestions, and — after you confirm — the run itself. The setup is\ndriven through the real flow for one task in each family (tabular, image, text)\nso the task-specific questions are visible; each core question prints as a\n`Step N of 4 · …` header, the task-specific ones (the label column, and extras\nlike resolution or schema) as their own header, the\nsupporting line beneath it, and the `?` line shows your answer. The run (shown\nonce, for tabular) is the three steps + the final summary as the CLI renders\nthem. Values passed as flags (--as, --task, a path, …) pre-fill the matching\nquestions rather than skipping them — guided mode always asks. The\nother tasks' extra questions (keypoints, label policy, time column),\nself-supervised text (which skips the label question), and the failure-summary\nwordings are in zz-all-strings.golden. The raw ingestor stream the CLI streams\nthrough (MySQL waits, the 📊 banner, per-validator lines) is the engine's own\nstdout — not CLI copy — so it isn't shown. (`tb ingest` is a hidden deprecated\nalias; `push` is a deprecated alias of the verb.)", []run{ {"tb data ingest # guided · tabular classification", tabularIngest}, {"tb data ingest # guided · image classification", imageIngest}, diff --git a/internal/cli/data_ingest_cmd.go b/internal/cli/data_ingest_cmd.go index 0f3d854..b1ab3f2 100644 --- a/internal/cli/data_ingest_cmd.go +++ b/internal/cli/data_ingest_cmd.go @@ -187,7 +187,6 @@ Exit codes: // Dropping --task's old image_classification default means an // unset task now drives the picker (TTY) or a clear error // (non-interactive), never a silent image assumption. - taskSet := cmd.Flags().Changed("task") || cmd.Flags().Changed("category") // Record whether --number-of-keypoints was explicitly passed, so // the keypoint set-vs-unset message (#76b) can distinguish an // explicit zero value from an unset flag (both look like the Go @@ -240,7 +239,6 @@ Exit codes: Printer: printer, Interactive: interactive, Prompter: pr, - TaskSet: taskSet, ChangedFlags: changedFlags, OutputJSON: outputJSON, JSONOut: jsonOut, @@ -337,15 +335,13 @@ type runDataIngestArgs struct { // the RunE from the persistent --plain flag (see printerFor). Printer *ui.Printer - // Interactive guided mode (#28). When Interactive is true, - // runDataIngest prompts (via Prompter) for any missing core inputs - // before validation. TaskSet records whether the task was passed - // explicitly (via --task or the hidden --category alias); an unset - // task drives the picker rather than assuming a default. Prompter is - // nil off a TTY / --no-input. + // Interactive guided mode (#28). When Interactive is true, runDataIngest + // walks every question relevant to the chosen task (via Prompter) before + // validation, pre-filling each from whatever arrived on the command line + // (#509). Prompter is nil off a TTY / --no-input, which is what keeps + // scripts flag-only. Interactive bool Prompter prompter - TaskSet bool // ReviewShown records whether the guided flow rendered the pre-confirm // Review (it only does when it actually prompted for something). It gates diff --git a/internal/cli/data_ingest_local.go b/internal/cli/data_ingest_local.go index a28436b..d977849 100644 --- a/internal/cli/data_ingest_local.go +++ b/internal/cli/data_ingest_local.go @@ -100,7 +100,7 @@ func resolveLocalInput(out, errOut io.Writer, a *runDataIngestArgs) (layout *pus // validation. Flags already provided win; non-TTY / --no-input // leaves Prompter nil and skips straight to the flag-only path. if a.Interactive && a.Prompter != nil { - if err := runInteractive(a.Printer, a.Prompter, a, a.TaskSet); err != nil { + if err := runInteractive(a.Printer, a.Prompter, a); err != nil { if errors.Is(err, errInteractiveCancelled) { // cleanCancel prints the shared note and returns the clean exit. return nil, nil, nil, true, cleanCancel(a.Printer, "nothing was ingested.") @@ -201,55 +201,18 @@ func resolveLocalInput(out, errOut io.Writer, a *runDataIngestArgs) (layout *pus 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 nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( - "%s is image tasks only; it doesn't apply to task %q", - f.name, a.Spec.Category)} - } - } - } - - // Task-scoped flags. Like --target-size/--min-size above, each of these is - // read only inside the one category branch that consumes it, so passing one - // on a task that doesn't use it silently dropped the value — and the user's - // intent — with no error, even though the help text says each is scoped. - // Reject a misapplied flag explicitly so it fails fast instead of being - // ignored (the scope mirrors spec.go's build gates exactly). - if a.SchemaFlag != "" && !push.IsTabular(a.Spec.Category) { - return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( - "--schema is tabular/time-series tasks only; it doesn't apply to task %q", a.Spec.Category)} - } - if a.Spec.LabelPolicy != "" && !push.IsRegressionClass(a.Spec.Category) { - return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( - "--label-policy is regression-class tasks only (tabular_regression, "+ - "time_series_forecasting, time_to_event_prediction); it doesn't apply to task %q", - a.Spec.Category)} - } - if a.Spec.TimeColumn != "" && a.Spec.Category != "time_to_event_prediction" { - return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( - "--time-column is time_to_event_prediction only; it doesn't apply to task %q", a.Spec.Category)} - } - if a.Spec.NumberOfKeypoints != 0 && a.Spec.Category != "keypoint_detection" { - return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( - "--number-of-keypoints is keypoint_detection only; it doesn't apply to task %q", a.Spec.Category)} - } - // --label-column is meaningless for self-supervised text (the label is the - // text itself); buildText drops it, so accepting it silently discarded the - // user's value and the review echoed a column that never shipped. - if a.Spec.LabelColumn != "" && push.SelfSupervisedText(a.Spec.Category) { - return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( - "--label-column doesn't apply to task %q — it trains on the text itself, with no label column", - a.Spec.Category)} + // Task-scoped flags. Each of these is read only inside the one category + // branch that consumes it, so passing one on a task that doesn't use it + // silently dropped the value — and the user's intent — with no error, even + // though the help text says each is scoped. Reject a misapplied flag + // explicitly so it fails fast instead of being ignored (the scope mirrors + // spec.go's build gates exactly). + // + // The scopes themselves live in task_scope.go, shared with the guided + // flow's post-picker reset: both need the same answer to "does this task + // use this value?", and two copies of that answer would drift silently. + if err := rejectMisappliedTaskValues(a); err != nil { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: err} } // 3. Walk the local directory FIRST (local "fail fast"), dispatched diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index 82dbc8a..bf3d05c 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -178,17 +178,40 @@ func isInteractiveTTY() bool { return term.IsTerminal(int(os.Stdin.Fd())) && term.IsTerminal(int(os.Stdout.Fd())) } -// runInteractive fills the gaps in a's core ingest fields by prompting, -// data-first (RFC-0002 §12.1): intent → name → path → task → task-specific -// questions → review. It only prompts for what's still missing, so flags -// the user already passed win. taskSet says whether the task was passed -// explicitly (via --task or the hidden --category alias); when it wasn't, -// the family is sniffed from the data the user pointed at (echoed back, or -// asked plainly when ambiguous) and only that family's tasks are offered. +// defaultInOptions returns want when it is one of options, otherwise fallback. +// A survey.Select whose Default is not among its Options aborts on a real +// terminal ("default value … not found in options"), so any command-line value +// used to PRE-FILL a Select must be validated against that Select's options +// first — a typo like --intent training or --label-policy buckets would +// otherwise crash the prompt survey can't draw. When the supplied value isn't a +// real option the question is still ASKED; the prompt just opens on the +// sensible fallback. This is the guard pickTask has always applied, factored out +// so every supplied-default Select shares one implementation. +func defaultInOptions(want string, options []string, fallback string) string { + for _, o := range options { + if o == want { + return want + } + } + return fallback +} + +// runInteractive walks a's core ingest fields by prompting, data-first +// (RFC-0002 §12.1): intent → name → path → task → task-specific questions → +// review. +// +// It asks EVERY question relevant to the chosen task (#509). A value the user +// passed on the command line pre-fills the answer — Enter accepts it — but never +// suppresses the question. Which questions are relevant still depends on the +// task picked at step 4; that is the only gate. +// +// When no task was supplied, the family is sniffed from the data the user +// pointed at (echoed back, or asked plainly when ambiguous) and only that +// family's tasks are offered. When one WAS supplied it selects the family +// directly, so the user's own answer is always among the options. // // Mutates a through the pointer. -func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bool) error { - prompted := false +func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs) error { // The guided flow is a four-step setup: intent → name → path → task. Each // question prints as its own step header (PromptStep), with any supporting @@ -206,37 +229,58 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo // blank sits directly between header and prompt. A result that belongs to an // answer (the sniff echo) attaches to it with no blank. + // Guided mode ASKS. A value that arrived on the command line becomes the + // prompt's DEFAULT — never a reason to skip the question (#509). + // + // It used to skip: each step was wrapped in `if == ""`. That turned a + // path which had merely gone stale into a dead end — the user was sitting at + // a prompt, ready to answer, and instead got "no such file or directory" with + // no way to correct it. Data moves between runs; a name or a task does not + // stop being true, but the flow should not have to reason about which is + // which. So: always ask, pre-filled, Enter accepts. + // + // Only the TASK gate survives, one level down — which questions apply still + // depends on the task chosen at step 4 (self-supervised text has no label and + // no extras). That gate is about relevance, not about what the user typed. + // + // Non-interactive is untouched: runInteractive is only reached on a TTY + // without --no-input/--output-json, so scripts still take flags silently. + // Step 1 — intent: what this data is for. - if a.Spec.Intent == "" { + { + opts := []string{"train", "test"} + // A supplied --intent pre-fills the prompt, but only when it names a real + // option: a typo would otherwise crash survey.Select on a TTY (see + // defaultInOptions). Unknown value → fall back to "train" and still ask. + def := defaultInOptions(a.Spec.Intent, opts, "train") p.PromptStep(1, 4, "Do you want to ingest training or test data?") p.Newline() ans, err := pr.Select("Do you want to ingest training or test data?", "which split this data is", - []string{"train", "test"}, "train") + opts, def) if err != nil { return err } a.Spec.Intent = ans - prompted = true } - // Step 2 — name. No auto-fill; the character rules surface only if the - // name is rejected (see ValidateTableName), so the prompt stays clean. - if a.Spec.Table == "" { + // Step 2 — name. The character rules surface only if the name is rejected + // (see ValidateTableName), so the prompt stays clean. + { p.PromptStep(2, 4, "Please name the dataset.") p.Newline() ans, err := pr.Input("Please name the dataset.", - "letters, digits, and underscores — no hyphens or spaces, use _; start with a letter or underscore e.g. churn_train", "", + "letters, digits, and underscores — no hyphens or spaces, use _; start with a letter or underscore e.g. churn_train", + a.Spec.Table, push.ValidateTableName) if err != nil { return err } a.Spec.Table = ans - prompted = true } // Step 3 — path. Show what "file or folder" means per modality, then // detect the family from the layout and echo it back. - if a.LocalPath == "" { + { p.PromptStep(3, 4, "Where is your data?") p.Newline() exTab, exImg, exTxt := datasetPathExamples(runtime.GOOS) @@ -245,7 +289,8 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo p.Infof("Images a folder with labels.csv + images/ e.g. %s", exImg) p.Infof("Text a folder with labels.csv + texts/ e.g. %s", exTxt) p.Newline() - ans, err := pr.Input("Where is your data?", fmt.Sprintf("e.g. %s or %s", exTab, exImg), "", validateDatasetPath) + ans, err := pr.Input("Where is your data?", fmt.Sprintf("e.g. %s or %s", exTab, exImg), + a.LocalPath, validateDatasetPath) if err != nil { return err } @@ -259,7 +304,6 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo // otherwise become part of the path (#386). validateDatasetPath applies // the same canonicalization, so the re-prompt guard stays consistent. a.LocalPath = dequotePath(ans) - prompted = true } // Expand a leading ~ now so the family sniff + label-header preview read // the real path; runDataIngest's own expandHome then no-ops. @@ -275,33 +319,53 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo return err } - // (d) task — family-scoped. An explicit --task wins and skips both the - // sniff and the picker (§5.1). Otherwise the family is sniffed from the - // layout (and echoed), or asked plainly when the layout is ambiguous, - // and then only that family's tasks are offered. - if !taskSet { - fam, err := resolveFamily(p, pr, a.LocalPath) - if err != nil { - return err + // (d) task — family-scoped, and always asked (#509). + // + // An explicit --task no longer skips the picker; it selects the family and + // becomes the pre-selected option. Taking the family from the SUPPLIED task + // rather than the sniff matters: the two can disagree (a tabular task passed + // against a folder that sniffs image), and scoping the list to the sniffed + // family would leave the user's own answer missing from its own default. + // With no task supplied, the family is sniffed from the layout (and echoed), + // or asked plainly when the layout is ambiguous — unchanged. + { + var fam push.Family + if spec, ok := push.Lookup(a.Spec.Category); ok { + fam = spec.Family + } else { + f, err := resolveFamily(p, pr, a.LocalPath) + if err != nil { + return err + } + fam = f } - id, err := pickTask(p, pr, fam) + supplied := a.Spec.Category + id, err := pickTask(p, pr, fam, supplied) if err != nil { return err } a.Spec.Category = id - prompted = true + // What the user just walked away from goes with it. A run started as + // `--task time_to_event_prediction --time-column t` that picks + // tabular_classification here would otherwise carry TimeColumn through: + // no prompt asks about it (it is time_to_event_prediction-only), Review + // shows it anyway, and the run dies AFTER the confirm blaming a flag the + // user just walked away from. Scoped to the CHANGE, so a flag that was + // misapplied on the command line survives to be rejected. + dropValuesLeftBehindByATaskChange(a, supplied) } // (e) task-specific questions, including the label column. - cp, err := promptCategorySpecific(p, pr, a) - if err != nil { + if _, err := promptCategorySpecific(p, pr, a); err != nil { return err } - prompted = prompted || cp - // (f) review + single confirm. Only when we actually prompted something - // — an ingest fully specified by flags (on a TTY) isn't nagged. - if prompted { + // (f) review + single confirm — unconditional now (#509). It used to be + // gated on "did we actually prompt anything", so an ingest fully specified + // by flags wasn't nagged. Guided mode always prompts now, so that gate can + // only ever be true; keeping it would be a condition that reads as a choice + // while having none. + { renderReview(p, a) a.ReviewShown = true // No header here: Confirm keeps its own label ("Proceed with the @@ -354,7 +418,7 @@ func resolveFamily(p *ui.Printer, pr prompter, path string) (push.Family, error) // task_id", split into Available now and (greyed) Not yet in the CLI — // and asks the user to pick one of the available ones. It never shows the // flat 15-item wall: only this family's tasks appear (§7). -func pickTask(p *ui.Printer, pr prompter, fam push.Family) (string, error) { +func pickTask(p *ui.Printer, pr prompter, fam push.Family, want string) (string, error) { var available, pending []push.CategorySpec for _, s := range push.CategoriesByFamily(fam) { if s.CLISupported { @@ -406,7 +470,12 @@ func pickTask(p *ui.Printer, pr prompter, fam push.Family) (string, error) { for i, s := range available { opts[i] = s.ID } - ans, err := pr.Select("Which task?", "pick the task this data is for", opts, opts[0]) + // def pre-selects a task the user already named (#509), honoured only when it + // is actually in this family's list (defaultInOptions) — survey would + // otherwise render a default the user cannot see, and an Enter on it would be + // unexplainable. + def := defaultInOptions(want, opts, opts[0]) + ans, err := pr.Select("Which task?", "pick the task this data is for", opts, def) if err != nil { return "", err } @@ -419,10 +488,11 @@ func pickTask(p *ui.Printer, pr prompter, fam push.Family) (string, error) { } // promptCategorySpecific prompts for the inputs a particular task needs -// beyond the core fields, filling only the gaps. The label column comes -// first (it's the one question every non-self-supervised task shares), -// then the family-specific extras. Returns whether it prompted anything -// (so the caller knows to show the confirm). +// beyond the core fields. Like the core steps (#509), each question is always +// asked with any supplied value pre-filled as the default — never skipped. The +// label column comes first (the one question every non-self-supervised task +// shares), then the family-specific extras. Returns whether it prompted anything +// (retained for the caller; the confirm is unconditional now). func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (bool, error) { cat := a.Spec.Category prompted := false @@ -437,7 +507,12 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b // (data-ingestors#340) that free-typing "Label" against a "label" header // would cause. Wording is per-task: a class to sort into vs a numeric value // to predict (§8). - if !push.SelfSupervisedText(cat) && a.Spec.LabelColumn == "" { + // + // A supplied --label-column pre-fills the pick (like every other value under + // #509) — it no longer SKIPS the question, so a wrong or mistyped column can + // be corrected here the same way a stale path can. Only self-supervised text + // (no label at all) still bypasses it. + if !push.SelfSupervisedText(cat) { question := "Which column holds the label?" desc := "The answer the model learns to produce — for classification, the class. e.g. diagnosis, churned" if push.IsRegressionClass(cat) { @@ -448,7 +523,7 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b p.Newline() p.Hintf("%s", desc) p.Newline() - ans, err := promptLabelColumn(pr, cat, a.LocalPath, question) + ans, err := promptLabelColumn(pr, cat, a.LocalPath, question, a.Spec.LabelColumn) if err != nil { return prompted, err } @@ -461,13 +536,17 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b // depends on the task. switch { case push.IsImage(cat): - if cat == "keypoint_detection" && a.Spec.NumberOfKeypoints <= 0 { + if cat == "keypoint_detection" { p.Section("How many keypoints per sample?") p.Newline() p.Hintf("The number of landmark points each sample is annotated with — dataset-specific. e.g. 17 for COCO human pose") p.Newline() + kpDef := "" + if a.Spec.NumberOfKeypoints > 0 { + kpDef = strconv.Itoa(a.Spec.NumberOfKeypoints) + } ans, err := pr.Input("How many keypoints per sample?", - "e.g. 17 for COCO pose", "", validatePositiveInt) + "e.g. 17 for COCO pose", kpDef, validatePositiveInt) if err != nil { return prompted, err } @@ -475,13 +554,13 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b a.Spec.NumberOfKeypoints = n prompted = true } - if a.TargetSizeFlag == "" { + { p.Section("Image resolution") p.Newline() p.Hintf("The size your images already are, as WxH — tracebloc checks every image matches and never resizes. Press Enter to read it from your first image. e.g. 224x224") p.Newline() ans, err := pr.Input("Image resolution", - "the size your images already are; tracebloc checks it, it never resizes", "", + "the size your images already are; tracebloc checks it, it never resizes", a.TargetSizeFlag, validateOptionalTargetSize) if err != nil { return prompted, err @@ -490,38 +569,46 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b prompted = true } case push.IsTabular(cat): - if a.SchemaFlag == "" { + { p.Section("Column types") p.Newline() p.Hintf("We infer each column's type from your CSV. Press Enter to accept, or type overrides like age:INT,price:FLOAT.") p.Newline() - ans, err := pr.Input("Column types", "e.g. age:INT,price:FLOAT", "", validateOptionalSchema) + ans, err := pr.Input("Column types", "e.g. age:INT,price:FLOAT", a.SchemaFlag, validateOptionalSchema) if err != nil { return prompted, err } a.SchemaFlag = strings.TrimSpace(ans) prompted = true } - if push.IsRegressionClass(cat) && a.Spec.LabelPolicy == "" { + if push.IsRegressionClass(cat) { p.Section("Label policy") p.Newline() p.Hintf("Regression targets are continuous. 'bucket' groups them into ranges before they leave the cluster; 'passthrough' keeps raw values.") p.Newline() + opts := []string{"bucket", "passthrough"} + // A supplied --label-policy pre-fills only when valid; a typo like + // "buckets" would otherwise abort the Select on a TTY (defaultInOptions). + lpDef := defaultInOptions(a.Spec.LabelPolicy, opts, "bucket") ans, err := pr.Select("Label policy", "bucket bins the target before it leaves the cluster", - []string{"bucket", "passthrough"}, "bucket") + opts, lpDef) if err != nil { return prompted, err } a.Spec.LabelPolicy = ans prompted = true } - if cat == "time_to_event_prediction" && a.Spec.TimeColumn == "" { + if cat == "time_to_event_prediction" { p.Section("Time column") p.Newline() p.Hintf("The column holding the duration / time-to-event. e.g. time, tenure_days") p.Newline() - ans, err := pr.Input("Time column", "the duration/time column name", "time", nil) + tcDef := a.Spec.TimeColumn + if tcDef == "" { + tcDef = "time" + } + ans, err := pr.Input("Time column", "the duration/time column name", tcDef, nil) if err != nil { return prompted, err } @@ -533,17 +620,24 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b } // promptLabelColumn asks for the label/target column. When the CSV header -// can be read, it offers those columns as an exact-match SELECT (defaulting -// to a column literally named "label" if present); otherwise — the header -// isn't readable yet — it falls back to free text so the flow never stalls. -func promptLabelColumn(pr prompter, category, root, question string) (string, error) { +// can be read, it offers those columns as an exact-match SELECT — pre-selecting +// a supplied --label-column when it names a real header, else a column literally +// named "label" if present; otherwise — the header isn't readable yet — it falls +// back to free text pre-filled with the supplied value so the flow never stalls. +// +// supplied is guarded through defaultInOptions before it reaches survey.Select: +// a mistyped --label-column that is not one of the real headers would otherwise +// abort the prompt on a TTY, the same default-not-in-options crash guarded +// everywhere else in the guided flow. +func promptLabelColumn(pr prompter, category, root, question, supplied string) (string, error) { headers, err := push.PreviewLabelHeaders(category, root) if err == nil && len(headers) > 0 { ans, serr := pr.Select(question, - "pick the label/target column from your CSV header", headers, defaultLabelChoice(headers)) + "pick the label/target column from your CSV header", headers, + defaultInOptions(supplied, headers, defaultLabelChoice(headers))) return strings.TrimSpace(ans), serr } - ans, ierr := pr.Input(question, "the label/target column name", "", nil) + ans, ierr := pr.Input(question, "the label/target column name", supplied, nil) return strings.TrimSpace(ans), ierr } diff --git a/internal/cli/interactive_test.go b/internal/cli/interactive_test.go index 548fc66..56a5ff2 100644 --- a/internal/cli/interactive_test.go +++ b/internal/cli/interactive_test.go @@ -3,8 +3,10 @@ package cli import ( "bytes" "errors" + "fmt" "os" "path/filepath" + "slices" "strings" "testing" @@ -41,7 +43,17 @@ func (f *fakePrompter) Input(label, _ /*help*/, def string, validate func(string return ans, nil } -func (f *fakePrompter) Select(label, _ /*help*/ string, _ []string, def string) (string, error) { +func (f *fakePrompter) Select(label, _ /*help*/ string, options []string, def string) (string, error) { + // Honour survey.Select's real contract: a non-empty Default that is not one + // of the Options aborts the prompt on a terminal ("default value … not found + // in options"). The original double ignored Options entirely, so a Select + // whose pre-filled default came from a mistyped flag stayed green here while + // crashing on a real TTY (PR #505). Validating the default in the double is + // what connects these tests to that failure — an unguarded supplied default + // now reddens instead of passing. + if def != "" && !slices.Contains(options, def) { + return "", fmt.Errorf("default value %q not found in options %v", def, options) + } return f.answer(label, def), nil } @@ -110,7 +122,7 @@ func TestRunInteractive_PromptOrder(t *testing.T) { "Which column holds the label?": "churned", }} a := &runDataIngestArgs{} - if err := runInteractive(discardPrinter(), f, a, false /*taskSet*/); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } @@ -146,7 +158,7 @@ func TestRunInteractive_PathPromptCopyIsFileOrFolder(t *testing.T) { "Which column holds the label?": "churned", }} a := &runDataIngestArgs{} - if err := runInteractive(discardPrinter(), f, a, false /*taskSet*/); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } found := false @@ -174,7 +186,7 @@ func TestRunInteractive_SniffEchoesFamily(t *testing.T) { a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{Intent: "train"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - if err := runInteractive(p, f, a, false); err != nil { + if err := runInteractive(p, f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if !strings.Contains(buf.String(), "Found a CSV table") { @@ -201,7 +213,7 @@ func TestRunInteractive_SniffIsHintNotLock(t *testing.T) { "Which column holds the label?": "label", }} a := &runDataIngestArgs{LocalPath: empty, Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if !contains(f.asked, "What kind of data is this?") { @@ -263,9 +275,10 @@ func TestResolveFamily_SurfacesMiscasedHint(t *testing.T) { } } -// TestRunInteractive_ExplicitTaskSkipsSniff: an explicit --task wins — no -// sniff echo, no family question, no task picker. -func TestRunInteractive_ExplicitTaskSkipsSniff(t *testing.T) { +// TestRunInteractive_ExplicitTaskStillAsks: an explicit --task no longer skips +// the picker (#509) — it PRE-SELECTS. It does still settle the family directly, +// so the sniff is unnecessary and the user's own task is among the options. +func TestRunInteractive_ExplicitTaskStillAsks(t *testing.T) { dir := tabularDir(t) f := &fakePrompter{answers: map[string]string{ "Please name the dataset.": "t", @@ -277,16 +290,26 @@ func TestRunInteractive_ExplicitTaskSkipsSniff(t *testing.T) { } var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - if err := runInteractive(p, f, a, true /*taskSet*/); err != nil { + if err := runInteractive(p, f, a); err != nil { t.Fatalf("runInteractive: %v", err) } + if !slices.Contains(f.asked, "Which task?") { + t.Errorf("the task question must still be asked; asked: %v", f.asked) + } + // The family came from the supplied task, so the layout sniff is not needed + // and its echo must not appear. for _, l := range f.asked { - if l == "Which task?" || l == "What kind of data is this?" { - t.Errorf("explicit --task must skip the picker/sniff; asked %q", l) + if l == "What kind of data is this?" { + t.Errorf("a supplied task settles the family; must not ask %q", l) } } if strings.Contains(buf.String(), "Found a CSV table") { - t.Errorf("explicit --task must not echo a sniff") + t.Errorf("a supplied task must not echo a sniff") + } + // Unanswered by the fake, so it returns the prompt's default — proving the + // supplied task was pre-selected rather than discarded. + if a.Spec.Category != "tabular_classification" { + t.Errorf("Category = %q, want the supplied task pre-selected", a.Spec.Category) } } @@ -301,7 +324,7 @@ func TestPickTask_FamilyScoped(t *testing.T) { f := &fakePrompter{answers: map[string]string{"Which task?": "text_classification"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - id, err := pickTask(p, f, push.FamilyText) + id, err := pickTask(p, f, push.FamilyText, "") if err != nil { t.Fatalf("pickTask: %v", err) } @@ -341,7 +364,7 @@ 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)) - if _, err := pickTask(p, f, push.FamilyImage); err != nil { + if _, err := pickTask(p, f, push.FamilyImage, ""); err != nil { t.Fatalf("pickTask: %v", err) } out := buf.String() @@ -367,7 +390,7 @@ func TestPickTask_TabularGloss(t *testing.T) { f := &fakePrompter{answers: map[string]string{"Which task?": "time_to_event_prediction"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - id, err := pickTask(p, f, push.FamilyTabular) + id, err := pickTask(p, f, push.FamilyTabular, "") if err != nil { t.Fatalf("pickTask: %v", err) } @@ -391,7 +414,7 @@ func TestRunInteractive_LabelSelectFromHeaders(t *testing.T) { "Which column holds the label?": "income", }} a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.Spec.LabelColumn != "income" { @@ -410,7 +433,7 @@ func TestRunInteractive_RegressionLabelWording(t *testing.T) { LocalPath: dir, Spec: push.SpecArgs{Category: "tabular_regression", Table: "t", Intent: "train"}, } - if err := runInteractive(discardPrinter(), f, a, true /*taskSet*/); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if !contains(f.asked, "Which column holds the value to predict?") { @@ -436,7 +459,7 @@ func TestRunInteractive_LabelFreeTextFallback(t *testing.T) { LocalPath: empty, Spec: push.SpecArgs{Category: "image_classification", Table: "t", Intent: "train"}, } - if err := runInteractive(discardPrinter(), f, a, true); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.Spec.LabelColumn != "my_label" { @@ -453,7 +476,7 @@ func TestRunInteractive_MLMSkipsLabel(t *testing.T) { "Which task?": "masked_language_modeling", }} a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } for _, l := range f.asked { @@ -466,9 +489,15 @@ func TestRunInteractive_MLMSkipsLabel(t *testing.T) { } } -// TestRunInteractive_SkipsProvidedValues: flags already set (and an -// explicit --task) mean nothing is prompted. -func TestRunInteractive_SkipsProvidedValues(t *testing.T) { +// TestRunInteractive_AsksEvenWhenFullySpecified is the inverse of the rule this +// replaced (#509). Guided mode used to prompt for nothing when every value +// arrived on the command line — which is what turned a path that had merely gone +// stale into a dead end, with the user sitting at a prompt that never came. +// +// Every core question must now be asked, and every supplied value must survive +// as the default: the fake answers nothing, so each field keeping its original +// value proves the pre-fill rather than a re-entered answer. +func TestRunInteractive_AsksEvenWhenFullySpecified(t *testing.T) { dir := textDirLayout(t) f := &fakePrompter{answers: map[string]string{}} a := &runDataIngestArgs{ @@ -477,11 +506,226 @@ func TestRunInteractive_SkipsProvidedValues(t *testing.T) { Category: "text_classification", Table: "t", Intent: "train", LabelColumn: "label", }, } - if err := runInteractive(discardPrinter(), f, a, true /*taskSet*/); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + for _, want := range []string{ + "Do you want to ingest training or test data?", + "Please name the dataset.", + "Where is your data?", + "Which task?", + } { + if !slices.Contains(f.asked, want) { + t.Errorf("guided mode must ask %q even when it was supplied; asked: %v", want, f.asked) + } + } + if a.Spec.Intent != "train" || a.Spec.Table != "t" || + a.Spec.Category != "text_classification" || a.LocalPath != dir { + t.Errorf("supplied values must survive as prompt defaults, got intent=%q table=%q category=%q path=%q", + a.Spec.Intent, a.Spec.Table, a.Spec.Category, a.LocalPath) + } +} + +// With nothing supplied, each question must still fall back to the value it +// always defaulted to. This pins behaviourally what the string-index backstop +// used to pin textually: moving those defaults out of inline arguments and into +// variables (so a supplied value can take their place) removed "train", +// "bucket" and "time" from zz-all-strings.golden. +func TestRunInteractive_UnsuppliedDefaultsUnchanged(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "time_to_event_prediction", + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{LocalPath: dir} + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.Intent != "train" { + t.Errorf("Intent = %q, want the unchanged \"train\" fallback", a.Spec.Intent) + } + if a.Spec.TimeColumn != "time" { + t.Errorf("TimeColumn = %q, want the unchanged \"time\" fallback", a.Spec.TimeColumn) + } +} + +func TestRunInteractive_LabelPolicyDefaultUnchanged(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "tabular_regression", + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{LocalPath: dir} + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.LabelPolicy != "bucket" { + t.Errorf("LabelPolicy = %q, want the unchanged \"bucket\" fallback", a.Spec.LabelPolicy) + } +} + +// A supplied value must be offered back as the DEFAULT, not silently replaced by +// the flow's own fallback — the specific regression that would make "always ask" +// feel like "always retype". +func TestRunInteractive_SuppliedValuesArePrefilled(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "tabular_regression", Table: "prefilled_name", Intent: "test", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + // "test" must survive: the Select's own fallback is "train". + if a.Spec.Intent != "test" { + t.Errorf("Intent = %q, want the supplied \"test\" pre-selected (flow default is \"train\")", a.Spec.Intent) + } + if a.Spec.Table != "prefilled_name" { + t.Errorf("Table = %q, want the supplied name pre-filled", a.Spec.Table) + } + if a.LocalPath != dir { + t.Errorf("LocalPath = %q, want the supplied path pre-filled", a.LocalPath) + } +} + +// TestDefaultInOptions pins the guard that keeps a command-line value from +// crashing a survey.Select: a valid value passes through, an empty or unknown +// one drops to the fallback. Reverting the helper body to `return want` reddens +// the empty, typo and case-mismatch rows. +func TestDefaultInOptions(t *testing.T) { + opts := []string{"train", "test"} + cases := []struct { + name, supplied, fallback, want string + }{ + {"valid-supplied-passes-through", "test", "train", "test"}, + {"first-option-supplied", "train", "train", "train"}, + {"empty-uses-fallback", "", "train", "train"}, + {"unknown-typo-uses-fallback", "training", "train", "train"}, + {"case-mismatch-uses-fallback", "Train", "train", "train"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := defaultInOptions(tc.supplied, opts, tc.fallback); got != tc.want { + t.Errorf("defaultInOptions(%q, %v, %q) = %q, want %q", + tc.supplied, opts, tc.fallback, got, tc.want) + } + }) + } +} + +// TestRunInteractive_InvalidSuppliedSelectDefaultFallsBack: a mistyped --intent +// or --label-policy must not crash the guided prompt. survey.Select aborts when +// its Default is not one of the Options; guided mode pre-fills those Selects with +// the command-line value, so a typo like --intent training would open a prompt +// survey refuses to draw (#505, High). The value is guarded (defaultInOptions): +// an unknown supplied value drops back to the flow's own default, and the +// question is still ASKED. +// +// Mutation-proof: the strict fake Select errors on a default that isn't in its +// options, so reverting either guard makes runInteractive return that error and +// this test fails. +func TestRunInteractive_InvalidSuppliedSelectDefaultFallsBack(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Which column holds the value to predict?": "income", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "tabular_regression", + Table: "reg_train", + Intent: "training", // typo — not train/test + LabelPolicy: "buckets", // typo — not bucket/passthrough + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("a mistyped supplied Select default must not crash the prompt: %v", err) + } + // Typo'd values were dropped to the flow's own valid defaults (the fake + // returns the prompt default when nothing is scripted for that question). + if a.Spec.Intent != "train" { + t.Errorf("Intent = %q, want the \"train\" fallback after a typo'd --intent", a.Spec.Intent) + } + if a.Spec.LabelPolicy != "bucket" { + t.Errorf("LabelPolicy = %q, want the \"bucket\" fallback after a typo'd --label-policy", a.Spec.LabelPolicy) + } +} + +// TestRunInteractive_SuppliedLabelColumnStillAsks: a supplied --label-column no +// longer SKIPS the label question (#505, Medium) — it PRE-FILLS it, exactly like +// a stale path. The old gate (LabelColumn == "") left a user who mistyped the +// column with no way to correct it at the prompt; now the header-backed picker +// still opens, so re-answering wins. +// +// Mutation-proof: restoring the `&& a.Spec.LabelColumn == ""` gate skips the +// question — f.asked loses it AND the re-answer no longer takes — so both +// assertions redden. +func TestRunInteractive_SuppliedLabelColumnStillAsks(t *testing.T) { + dir := tabularDir(t) // header: age,income,churned + f := &fakePrompter{answers: map[string]string{ + // Re-answer the label with a different real column: only reachable if the + // question was actually asked rather than skipped by the supplied value. + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "tabular_classification", Table: "t", Intent: "train", + LabelColumn: "income", // supplied — must pre-fill, not skip + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } - if len(f.asked) != 0 { - t.Errorf("expected no prompts, but asked: %v", f.asked) + if !contains(f.asked, "Which column holds the label?") { + t.Errorf("a supplied --label-column must still ASK the label question; asked=%v", f.asked) + } + if a.Spec.LabelColumn != "churned" { + t.Errorf("LabelColumn = %q, want the re-answered \"churned\" (prompt was live, not skipped)", a.Spec.LabelColumn) + } +} + +// TestPromptLabelColumn_SuppliedDefaultPrefillsAndGuards drives the label picker +// directly: a supplied column that names a real header is pre-selected, while an +// empty or mistyped one falls back to the header-derived default without crashing +// the Select. It is the same defaultInOptions guard used for --intent, applied to +// the column the label question now pre-fills (#505). +// +// Mutation-proof: passing `supplied` straight to pr.Select (dropping the guard) +// makes the "mistyped" row error under the strict fake; not threading the value +// at all makes the "valid-supplied" row return the header default instead. +func TestPromptLabelColumn_SuppliedDefaultPrefillsAndGuards(t *testing.T) { + dir := tabularDir(t) // header: age,income,churned (no column named "label") + const cat = "tabular_classification" + const q = "Which column holds the label?" + + cases := []struct { + name, supplied, want string + }{ + // Nothing scripted → the strict fake returns the Select's default, so the + // asserted value IS whatever default promptLabelColumn passed in. + {"valid-supplied-is-preselected", "income", "income"}, + {"empty-supplied-uses-header-default", "", "age"}, // defaultLabelChoice → first header + {"mistyped-supplied-falls-back", "incom", "age"}, // guarded, no crash + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := &fakePrompter{answers: map[string]string{}} + got, err := promptLabelColumn(f, cat, dir, q, tc.supplied) + if err != nil { + t.Fatalf("promptLabelColumn(supplied=%q) errored (default not guarded into options?): %v", tc.supplied, err) + } + if got != tc.want { + t.Errorf("promptLabelColumn(supplied=%q) = %q, want %q", tc.supplied, got, tc.want) + } + }) } } @@ -497,7 +741,7 @@ func TestRunInteractive_Keypoint(t *testing.T) { LocalPath: dir, Spec: push.SpecArgs{Category: "keypoint_detection", Table: "kp_train", Intent: "train"}, } - if err := runInteractive(discardPrinter(), f, a, true); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.Spec.NumberOfKeypoints != 17 { @@ -520,7 +764,7 @@ func TestRunInteractive_TabularRegression(t *testing.T) { LocalPath: dir, Spec: push.SpecArgs{Category: "tabular_regression", Table: "reg_train", Intent: "train"}, } - if err := runInteractive(discardPrinter(), f, a, true); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.Spec.LabelPolicy != "passthrough" { @@ -544,7 +788,7 @@ func TestRunInteractive_Cancel(t *testing.T) { confirm: &no, } a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); !errors.Is(err, errInteractiveCancelled) { + if err := runInteractive(discardPrinter(), f, a); !errors.Is(err, errInteractiveCancelled) { t.Fatalf("err = %v, want errInteractiveCancelled", err) } } @@ -554,7 +798,7 @@ func TestRunInteractive_Cancel(t *testing.T) { func TestRunInteractive_RejectsBadName(t *testing.T) { f := &fakePrompter{answers: map[string]string{"Please name the dataset.": "../bad"}} a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, true); err == nil { + if err := runInteractive(discardPrinter(), f, a); err == nil { t.Fatal("expected an error for an invalid name, got nil") } } @@ -568,7 +812,7 @@ func TestRunInteractive_RejectsEmptyPath(t *testing.T) { "Where is your data?": " ", }} a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err == nil { + if err := runInteractive(discardPrinter(), f, a); err == nil { t.Fatal("expected an error for an empty dataset path, got nil") } } @@ -586,7 +830,7 @@ func TestRunInteractive_TrimsPath(t *testing.T) { "Which column holds the label?": "churned", }} a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.LocalPath != dir { @@ -612,7 +856,7 @@ func TestRunInteractive_ShowsExampleHints(t *testing.T) { a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - if err := runInteractive(p, f, a, false); err != nil { + if err := runInteractive(p, f, a); err != nil { t.Fatalf("runInteractive: %v", err) } for _, want := range []string{"~/data/patients.csv", "age:INT"} { diff --git a/internal/cli/task_scope.go b/internal/cli/task_scope.go new file mode 100644 index 0000000..ccf16a5 --- /dev/null +++ b/internal/cli/task_scope.go @@ -0,0 +1,184 @@ +package cli + +import ( + "errors" + "fmt" + + "github.com/tracebloc/cli/internal/push" +) + +// taskScopedValue is one value that only some tasks use — the flags each read +// inside a single category branch, so a value passed against a task that +// doesn't consume it would otherwise be silently dropped. +// +// The scope predicate lives HERE, once, because two callers need the same +// answer to "does this task use this value?": +// +// - the misapplied-flag guard in runDataIngestLocal, which rejects a value +// the chosen task cannot use, and +// - dropOutOfScopeTaskValues, which the guided flow runs after the task +// picker so a value the user is no longer choosing doesn't survive. +// +// Written as two copies of each predicate, they would agree today and drift on +// the next task added to a family — and drift here is invisible: both copies +// keep passing their own tests while disagreeing with each other. +type taskScopedValue struct { + // flag names the value as the user typed it, for the message. + flag string + // inScope answers whether this task consumes the value. + inScope func(category string) bool + // isSet reports whether the value is present at all. + isSet func(*runDataIngestArgs) bool + // set puts a representative value there. Only tests call it, and they call + // it to build states from the table rather than by hand — a hand-built + // fixture is one more copy of the scopes to drift. + set func(*runDataIngestArgs) + // clear returns the value to "not supplied". + clear func(*runDataIngestArgs) + // message is the whole rejection sentence, written out rather than + // composed, so the copy catalog (zz-all-strings.golden) shows a reviewer + // the exact string a user sees instead of a fragment. A test pins that each + // one names its own flag. + message func(category string) string +} + +// taskScopedValues is the whole set. Order fixes the order of rejection when +// more than one is misapplied, so the message a user sees is deterministic. +var taskScopedValues = []taskScopedValue{ + { + flag: "--target-size", + inScope: push.IsImage, + isSet: func(a *runDataIngestArgs) bool { return a.TargetSizeFlag != "" }, + clear: func(a *runDataIngestArgs) { a.TargetSizeFlag = "" }, + set: func(a *runDataIngestArgs) { a.TargetSizeFlag = "224x224" }, + message: func(cat string) string { + return fmt.Sprintf("--target-size is image tasks only; it doesn't apply to task %q", cat) + }, + }, + { + flag: "--min-size", + inScope: push.IsImage, + isSet: func(a *runDataIngestArgs) bool { return a.MinSizeFlag != "" }, + clear: func(a *runDataIngestArgs) { a.MinSizeFlag = "" }, + set: func(a *runDataIngestArgs) { a.MinSizeFlag = "32x32" }, + message: func(cat string) string { + return fmt.Sprintf("--min-size is image tasks only; it doesn't apply to task %q", cat) + }, + }, + { + flag: "--schema", + inScope: push.IsTabular, + isSet: func(a *runDataIngestArgs) bool { return a.SchemaFlag != "" }, + clear: func(a *runDataIngestArgs) { a.SchemaFlag = "" }, + set: func(a *runDataIngestArgs) { a.SchemaFlag = "age:INT" }, + message: func(cat string) string { + return fmt.Sprintf("--schema is tabular/time-series tasks only; it doesn't apply to task %q", cat) + }, + }, + { + flag: "--label-policy", + inScope: push.IsRegressionClass, + isSet: func(a *runDataIngestArgs) bool { return a.Spec.LabelPolicy != "" }, + clear: func(a *runDataIngestArgs) { a.Spec.LabelPolicy = "" }, + set: func(a *runDataIngestArgs) { a.Spec.LabelPolicy = "bucket" }, + message: func(cat string) string { + return fmt.Sprintf("--label-policy is regression-class tasks only (tabular_regression, "+ + "time_series_forecasting, time_to_event_prediction); it doesn't apply to task %q", cat) + }, + }, + { + flag: "--time-column", + inScope: func(cat string) bool { return cat == "time_to_event_prediction" }, + isSet: func(a *runDataIngestArgs) bool { return a.Spec.TimeColumn != "" }, + clear: func(a *runDataIngestArgs) { a.Spec.TimeColumn = "" }, + set: func(a *runDataIngestArgs) { a.Spec.TimeColumn = "t" }, + message: func(cat string) string { + return fmt.Sprintf("--time-column is time_to_event_prediction only; it doesn't apply to task %q", cat) + }, + }, + { + flag: "--number-of-keypoints", + inScope: func(cat string) bool { return cat == "keypoint_detection" }, + isSet: func(a *runDataIngestArgs) bool { return a.Spec.NumberOfKeypoints != 0 }, + clear: func(a *runDataIngestArgs) { a.Spec.NumberOfKeypoints = 0 }, + set: func(a *runDataIngestArgs) { a.Spec.NumberOfKeypoints = 17 }, + message: func(cat string) string { + return fmt.Sprintf("--number-of-keypoints is keypoint_detection only; it doesn't apply to task %q", cat) + }, + }, + { + // The one inverted scope: every task uses a label column EXCEPT + // self-supervised text, which trains on the text itself. buildText drops + // the value, so accepting it silently discarded the user's answer and + // the review echoed a column that never shipped. + flag: "--label-column", + inScope: func(cat string) bool { return !push.SelfSupervisedText(cat) }, + isSet: func(a *runDataIngestArgs) bool { return a.Spec.LabelColumn != "" }, + clear: func(a *runDataIngestArgs) { a.Spec.LabelColumn = "" }, + set: func(a *runDataIngestArgs) { a.Spec.LabelColumn = "label" }, + message: func(cat string) string { + return fmt.Sprintf("--label-column doesn't apply to task %q — it trains on the text itself, with no label column", cat) + }, + }, +} + +// rejectMisappliedTaskValues returns the first value present that the chosen +// task cannot use. Flag-only runs reach this with whatever the user typed; +// guided runs reach it after dropOutOfScopeTaskValues has already removed +// anything the user re-chose away from, so it can only fire on a real mistake. +func rejectMisappliedTaskValues(a *runDataIngestArgs) error { + for _, v := range taskScopedValues { + if v.isSet(a) && !v.inScope(a.Spec.Category) { + return errors.New(v.message(a.Spec.Category)) + } + } + return nil +} + +// dropValuesLeftBehindByATaskChange clears the task-scoped values that the +// task the user just CHOSE does not use, but the task they arrived with did. +// +// The guided flow calls this immediately after the picker. Without it, a run +// started as `--task time_to_event_prediction --time-column t` that picks +// tabular_classification keeps TimeColumn: the prompt for it never appears (it +// is time_to_event_prediction-only), Review shows it anyway, and the run dies +// AFTER the confirm blaming a flag the user just spent a prompt walking away +// from. Guided mode's promise is that the answers on screen are the run; a +// value no question asked about and no answer can reach is not one of them. +// +// It is scoped to what the CHANGE left behind — `v.inScope(from)` — and not to +// everything out of scope, because those are different sets and clearing the +// wrong one swallows a real mistake. `--task tabular_classification +// --time-column t` is misapplied on the command line: no task change walks away +// from it, so nothing here may clear it and rejectMisappliedTaskValues must +// still fire. Otherwise pressing Enter on the pre-selected task would silently +// drop the flag while the identical invocation under --no-input exits 2 — +// guided mode quietly meaning something different from the flags it echoes +// (Bugbot). +// +// A run with no --task supplied is not a change either: the user never declared +// a task to move away from, so every value stands or falls on the guard. +// +// Nor is a --task the registry does not recognize. A name nobody registered is +// not a task anyone walked away from, and treating it as one clears values on +// its behalf: every +// `inScope` predicate answers from the registry, so an unknown id lands on the +// default side of each one — `!SelfSupervisedText("tabular_classifier")` is +// true because the lookup misses, not because the task uses a label column. So +// `--task tabular_classifier --label-column x`, picking a self-supervised +// text task, silently dropped --label-column, and the guided flow runs BEFORE +// the category gate (data_ingest_local.go:103 vs :165), so the typo itself was +// never reported either — the picker had already overwritten it with a valid id. +// Two values lost, no message, where --no-input exits 2 on the same command. +// An unknown `from` is therefore the no-task case: clear nothing, and let +// rejectMisappliedTaskValues speak (Bugbot). +func dropValuesLeftBehindByATaskChange(a *runDataIngestArgs, from string) { + if from == "" || !push.IsKnown(from) || from == a.Spec.Category { + return + } + for _, v := range taskScopedValues { + if v.inScope(from) && !v.inScope(a.Spec.Category) { + v.clear(a) + } + } +} diff --git a/internal/cli/task_scope_test.go b/internal/cli/task_scope_test.go new file mode 100644 index 0000000..81fc78e --- /dev/null +++ b/internal/cli/task_scope_test.go @@ -0,0 +1,290 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/tracebloc/cli/internal/push" +) + +// A run started with task-scoped flags that then picks a DIFFERENT task at the +// guided prompt must not carry the old task's values through. Before the reset, +// `--task time_to_event_prediction --time-column t` + picking +// tabular_classification kept TimeColumn on the spec: no prompt asks about it +// (it is time_to_event_prediction-only), Review showed it anyway, and the run +// then died AFTER the confirm blaming a flag the user had just walked away from. +func TestGuided_TaskScopedValuesDoNotSurviveATaskChange(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "tabular_classification", + "Which column holds the label?": "churned", + }} + // A LEGAL starting state: every value here is in scope for the supplied + // task. Starting from an illegal one would test the wrong thing — a flag + // misapplied from the outset is the guard's job, not the reset's. + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "time_to_event_prediction", + TimeColumn: "t", + LabelPolicy: "bucket", + LabelColumn: "churned", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.TimeColumn != "" { + t.Errorf("TimeColumn = %q survived the change to tabular_classification", a.Spec.TimeColumn) + } + if a.Spec.LabelPolicy != "" { + t.Errorf("LabelPolicy = %q survived the change", a.Spec.LabelPolicy) + } + // Not a blanket wipe: the label column IS in scope for the new task, and + // the answer just given must stand. + if a.Spec.LabelColumn != "churned" { + t.Errorf("LabelColumn = %q, want the answer just given", a.Spec.LabelColumn) + } + if err := rejectMisappliedTaskValues(a); err != nil { + t.Errorf("the state left behind is one the guard rejects: %v", err) + } +} + +// The same within the image family, where the value dropped is a number rather +// than a string and two neighbouring flags must survive. +func TestGuided_KeypointsGoWhenTheTaskStopsUsingThem(t *testing.T) { + dir := imageDirLayout(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "image_classification", + "Which column holds the label?": "label", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + TargetSizeFlag: "224x224", + MinSizeFlag: "32x32", + Spec: push.SpecArgs{ + Category: "keypoint_detection", + NumberOfKeypoints: 17, + LabelColumn: "label", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.NumberOfKeypoints != 0 { + t.Errorf("NumberOfKeypoints = %d survived the change to image_classification", a.Spec.NumberOfKeypoints) + } + if a.TargetSizeFlag == "" || a.MinSizeFlag == "" { + t.Errorf("image flags were wiped though both tasks are image tasks: target=%q min=%q", + a.TargetSizeFlag, a.MinSizeFlag) + } +} + +// The reset must be a no-op when the picked task is the supplied one — the +// tests above cannot tell "cleared what the change left behind" from "cleared +// everything", and clearing everything would silently discard flags the user +// meant and the prompts pre-fill from. +func TestGuided_TaskScopedValuesSurviveWhenTheTaskIsUnchanged(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "time_to_event_prediction", + "Which column holds the value to predict?": "days", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "time_to_event_prediction", + TimeColumn: "tenure_days", + LabelPolicy: "passthrough", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.TimeColumn != "tenure_days" { + t.Errorf("TimeColumn = %q, want the supplied value to survive as the prompt default", a.Spec.TimeColumn) + } + if a.Spec.LabelPolicy != "passthrough" { + t.Errorf("LabelPolicy = %q, want the supplied value to survive", a.Spec.LabelPolicy) + } +} + +// Every row's rejection must still fire for a task outside its scope, and must +// name its own flag. Without the name check a row could carry another row's +// message — the table makes that a copy-paste away, and the resulting error +// would send the user to a flag they never passed. +func TestEveryTaskScopedValueRejectsOutOfScopeAndNamesItsFlag(t *testing.T) { + for _, v := range taskScopedValues { + t.Run(v.flag, func(t *testing.T) { + // Find a task this value does NOT apply to, from the real task list + // rather than a hand-picked one, so a scope widened later still + // finds its counterexample or fails loudly here. + var out string + for _, cat := range push.SupportedCategoryIDs() { + if !v.inScope(cat) { + out = cat + break + } + } + if out == "" { + t.Fatalf("%s applies to every supported task — it is not task-scoped", v.flag) + } + msg := v.message(out) + if !strings.Contains(msg, v.flag) { + t.Errorf("message %q does not name its own flag %s", msg, v.flag) + } + if !strings.Contains(msg, out) { + t.Errorf("message %q does not name the task it was rejected for", msg) + } + }) + } +} + +// A flag misapplied on the COMMAND LINE must still be rejected in guided mode. +// The first version of the reset cleared everything out of scope for the picked +// task, which also cleared a flag that was wrong from the start — so pressing +// Enter on the pre-selected task silently dropped it while the identical +// invocation under --no-input exited 2. Guided mode quietly meaning something +// different from the flags it echoes is worse than the bug it replaced. +func TestGuided_AMisappliedFlagIsStillRejectedWhenTheTaskIsKept(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "tabular_classification", + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{Category: "tabular_classification", TimeColumn: "t"}, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.TimeColumn != "t" { + t.Fatalf("TimeColumn = %q — the reset swallowed a flag no task change walked away from", a.Spec.TimeColumn) + } + if err := rejectMisappliedTaskValues(a); err == nil { + t.Error("the misapplied --time-column was accepted; --no-input would exit 2 on the same command line") + } +} + +// Same, with no --task supplied at all: the user never declared a task to move +// away from, so nothing was walked away from and the guard still speaks. +func TestGuided_AMisappliedFlagIsStillRejectedWhenNoTaskWasSupplied(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "tabular_classification", + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{TimeColumn: "t"}} + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if err := rejectMisappliedTaskValues(a); err == nil { + t.Error("a --time-column with no --task was silently dropped rather than rejected") + } +} + +// An UNRECOGNIZED --task is not a task the user walked away from, and this is the +// that made the reset silently eat two values at once. Every `inScope` predicate +// answers from the registry, so an unknown id lands on the DEFAULT side of each: +// `!SelfSupervisedText("tabular_classifier")` is true because the lookup +// misses. Pick a self-supervised text task and --label-column then reads as +// "in scope before, out of scope now" and gets cleared — so +// rejectMisappliedTaskValues finds nothing to complain about. The typo is gone +// too: the guided flow runs before the category gate +// (data_ingest_local.go:103 vs :165) and the picker has already overwritten it +// with a valid id, so the run proceeds as though the user typed neither flag, +// where --no-input exits 2 on the same command line. +func TestGuided_AnUnrecognizedTaskIsNotATaskChange(t *testing.T) { + dir := textDirLayout(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "mlm_train", + "Which task?": "masked_language_modeling", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + // A plausible wrong name (not tabular_classification): unknown to the + // registry, so SelfSupervisedText() misses and every inScope() defaults + // to true. Spelled correctly on purpose — `misspell` is a lint gate here. + Spec: push.SpecArgs{Category: "tabular_classifier", LabelColumn: "churned"}, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.LabelColumn != "churned" { + t.Fatalf("LabelColumn = %q — the reset cleared it on behalf of a task that does not exist", a.Spec.LabelColumn) + } + if err := rejectMisappliedTaskValues(a); err == nil { + t.Error("a --label-column misapplied alongside an unrecognized --task was accepted; --no-input would exit 2") + } +} + +// The edge Shujaat named on review: the user changes A -> B and the flag was +// invalid for BOTH. It is tempting to treat a re-pick as blanket permission to +// drop, but the flag was already wrong when they typed it and no answer they +// gave walks away from it — so it must still be rejected, exactly as it would +// be under --no-input. `v.inScope(from)` is what makes that true; a reset +// keyed only on the picked task would swallow it. +func TestGuided_AFlagInvalidForBothTasksIsStillRejected(t *testing.T) { + dir := imageDirLayout(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "image_classification", + "Which column holds the label?": "label", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "keypoint_detection", + LabelColumn: "label", + // --time-column applies to neither keypoint_detection nor + // image_classification. + TimeColumn: "t", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.TimeColumn != "t" { + t.Fatalf("TimeColumn = %q — a re-pick is not permission to drop a flag that was always wrong", a.Spec.TimeColumn) + } + if err := rejectMisappliedTaskValues(a); err == nil { + t.Error("the misapplied --time-column was accepted after a task change") + } +} + +// The property the shared predicate buys: from ANY legal starting state — the +// values in scope for the task the user arrived with — a change to ANY other +// task leaves a state the guard accepts. Both sides are built from the table, +// so a scope widened later is exercised here without editing this test. +func TestAnyLegalStateSurvivesAnyTaskChange(t *testing.T) { + cats := push.SupportedCategoryIDs() + for _, from := range cats { + for _, to := range cats { + if from == to { + continue + } + t.Run(from+"->"+to, func(t *testing.T) { + a := &runDataIngestArgs{Spec: push.SpecArgs{Category: from}} + for _, v := range taskScopedValues { + if v.inScope(from) { + v.set(a) + } + } + if err := rejectMisappliedTaskValues(a); err != nil { + t.Fatalf("the starting state is not legal, so this proves nothing: %v", err) + } + a.Spec.Category = to + dropValuesLeftBehindByATaskChange(a, from) + if err := rejectMisappliedTaskValues(a); err != nil { + t.Errorf("after the change the guard rejects: %v", err) + } + }) + } + } +} diff --git a/internal/cli/testdata/golden/01-data-ingest.golden b/internal/cli/testdata/golden/01-data-ingest.golden index 678512a..15f6427 100644 --- a/internal/cli/testdata/golden/01-data-ingest.golden +++ b/internal/cli/testdata/golden/01-data-ingest.golden @@ -9,7 +9,8 @@ so the task-specific questions are visible; each core question prints as a like resolution or schema) as their own header, the supporting line beneath it, and the `?` line shows your answer. The run (shown once, for tabular) is the three steps + the final summary as the CLI renders -them. Passing flags (--as, --task, a path, …) skips the matching questions. The +them. Values passed as flags (--as, --task, a path, …) pre-fill the matching +questions rather than skipping them — guided mode always asks. The other tasks' extra questions (keypoints, label policy, time column), self-supervised text (which skips the label question), and the failure-summary wordings are in zz-all-strings.golden. The raw ingestor stream the CLI streams diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 368f832..3dec5c0 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -44,7 +44,6 @@ screen. %s/%d are runtime placeholders. "%s has a header but no data rows (0 ingestable records). Add at least one data row and re-run." "%s is empty — add a header and at least one data row, then re-run" "%s is empty — no header row" -"%s is image tasks only; it doesn't apply to task %q" "%s of %s GiB" "%s of %s cores" "%s requires CLIENT_WRITE permission" @@ -78,10 +77,12 @@ screen. %s/%d are runtime placeholders. "-%02d" "--%s has no effect without --seal" "--label-column doesn't apply to task %q — it trains on the text itself, with no label column" +"--min-size is image tasks only; it doesn't apply to task %q" "--number-of-keypoints is keypoint_detection only; it doesn't apply to task %q" "--overwrite can't be combined with --idempotency-key: a reused key makes the cluster replay the previous run instead of ingesting the new data — after --overwrite's removal that would report success while loading nothing. Drop one of the two (a fresh per-run key is the default)." "--schema is empty; expected col:TYPE,col:TYPE,..." "--schema is tabular/time-series tasks only; it doesn't apply to task %q" +"--target-size is image tasks only; it doesn't apply to task %q" "--time-column is time_to_event_prediction only; it doesn't apply to task %q" "--timeout has no effect without --wait or --seal" "--wait and --seal are separate modes — run them one at a time" @@ -369,7 +370,6 @@ screen. %s/%d are runtime placeholders. "backend" "backend %s — requesting a device code …" "backfilling the cluster anchor onto the existing client: %w" -"bucket" "bucket bins the target before it leaves the cluster" "building SPDY transport: %w" "building rest config from kubeconfig: %w" @@ -606,7 +606,6 @@ screen. %s/%d are runtime placeholders. "the sign-in code expired — re-run `tracebloc login`" "the size your images already are; tracebloc checks it, it never resizes" "this machine has %s, but you asked for %s." -"time" "time column" "token saved to ~/.tracebloc (0600)" "total records" @@ -621,7 +620,6 @@ screen. %s/%d are runtime placeholders. "tracebloc's downloaded images" "tracebloc-doctor-%s.txt" "tracebloc-stage-%s-%s" -"train" "unavailable" "unknown command %q for %q" "upgrade didn't complete (%w). You can run the installer directly:\n %s"