diff --git a/internal/cli/coverage_test.go b/internal/cli/coverage_test.go index 586ee645..c176166b 100644 --- a/internal/cli/coverage_test.go +++ b/internal/cli/coverage_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "os" + "os/user" "path/filepath" "strings" "testing" @@ -173,7 +174,10 @@ func TestClassifyPushOutcome(t *testing.T) { func TestRunDatasetPush_OutputJSONEarlyFailureEmitsJSON(t *testing.T) { var jsonBuf, human bytes.Buffer a := runDataIngestArgs{ - LocalPath: "./x", + // A real path so the failure is the invalid table name (exit 2), not + // the earlier path-existence check (exit 3, #181) — this test pins the + // stdout-always-JSON contract on the table-validation failure. + LocalPath: t.TempDir(), Spec: push.SpecArgs{Table: "../bad", Category: "image_classification", Intent: "train"}, Printer: ui.New(&human, ui.WithColor(false)), OutputJSON: true, @@ -219,6 +223,44 @@ func TestExpandHome(t *testing.T) { } } +// TestExpandHome_NamedUser covers the #181 ~user form: "~user" and +// "~user/…" resolve under that user's home. We look up the CURRENT user by +// name so the test doesn't depend on a fixed account existing, and compare +// against os.UserHomeDir. An unknown ~user is left literal (the path- +// existence check surfaces it), which we also pin. +func TestExpandHome_NamedUser(t *testing.T) { + u, err := user.Current() + if err != nil || u.Username == "" { + t.Skipf("no current user: %v", err) + } + // user.Lookup must resolve the same account (it can differ from + // UserHomeDir on some CI images); skip if it doesn't rather than assert + // on an environment quirk. + looked, err := user.Lookup(u.Username) + if err != nil { + t.Skipf("user.Lookup(%q) unsupported here: %v", u.Username, err) + } + home := looked.HomeDir + + cases := []struct{ in, want string }{ + {"~" + u.Username, home}, + {"~" + u.Username + "/data", filepath.Join(home, "data")}, + {"~" + u.Username + "/a/b", filepath.Join(home, "a", "b")}, + } + for _, c := range cases { + if got := expandHome(c.in); got != c.want { + t.Errorf("expandHome(%q) = %q, want %q", c.in, got, c.want) + } + } + + // An unknown user can't be resolved: the literal is returned unchanged so + // the downstream path-existence check reports it plainly. + const unknown = "~nsuchuser-tracebloc-181/x" + if got := expandHome(unknown); got != unknown { + t.Errorf("expandHome(%q) = %q, want it left literal", unknown, got) + } +} + // TestExitError_Methods pins the exit-code carrier: Error() surfaces // the wrapped message (or a fallback when nil), and Code() returns the // process exit code main() propagates. diff --git a/internal/cli/data.go b/internal/cli/data.go index 89bdf348..6b066339 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -15,6 +15,7 @@ import ( "gopkg.in/yaml.v3" "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/pathutil" "github.com/tracebloc/cli/internal/push" "github.com/tracebloc/cli/internal/schema" "github.com/tracebloc/cli/internal/submit" @@ -44,6 +45,11 @@ submits the ingestion run, and watches it to completion (streaming logs + the final summary). ` + "`data validate`" + ` checks an ingest.yaml locally first. +What a dataset looks like depends on the task: + tabular / time-series — a .csv file, or a folder with one .csv + image — a folder with labels.csv + images/ + text — a folder with labels.csv + texts/ + ` + "`tracebloc cluster info`" + ` is the pre-flight you'd typically run before the first ingest.`, // A bare `tracebloc data` prints help; a mistyped subcommand errors with a @@ -58,7 +64,7 @@ before the first ingest.`, return cmd } -// newDataIngestCmd implements `tracebloc data ingest `. +// newDataIngestCmd implements `tracebloc data ingest `. // // Phase 3 scope (now complete across PR-a + PR-b): // @@ -133,7 +139,7 @@ func newDataIngestCmd() *cobra.Command { ) cmd := &cobra.Command{ - Use: "ingest ", + Use: "ingest ", Aliases: []string{"push"}, Short: "Ingest a local dataset into your workspace", Long: `Ingests a local dataset into your workspace's storage, @@ -143,14 +149,36 @@ infrastructure. Supports 9 tasks (image classification, object/keypoint detection, text classification, masked language modeling, and the tabular / time-series family); pick one with --task. -Expected local layout (image_classification shown): + is the data itself. What it looks like depends on the task: + + tabular / time-series — the dataset is a single CSV. Pass the .csv + file directly, or a folder holding exactly one .csv: + + churn.csv (the .csv file itself) + or + churn/ + data.csv (the one .csv in the folder) + + image (classification, object/keypoint detection) — a folder with + labels.csv + an images/ subfolder: + + cats_dogs/ + labels.csv (required) + images/ (required) + 001.jpg + ... - / - labels.csv (required) - images/ (required) - 001.jpg - 002.jpg - ... + text (classification, masked language modeling) — a folder with + labels.csv + a texts/ subfolder: + + reviews/ + labels.csv (required) + texts/ (required) + 001.txt + ... + +A bare .csv file is accepted only for the tabular / time-series family; +image and text datasets must be a folder. Accepted image extensions: .jpg, .jpeg, or .png (case-insensitive). All images in one dataset must share a single type — the cluster @@ -358,25 +386,32 @@ type runDataIngestArgs struct { ImageDigest string } -// expandHome expands a leading ~ or ~/… to $HOME, leaving every other -// path (relative, absolute, empty) untouched. It mirrors -// cluster.expandPath — kept as a small local copy rather than coupling -// the data path-handling to the cluster package's internals; if a -// third caller appears, promote both to a shared pathutil. +// expandHome expands a leading ~ (current user or ~user) to a home +// directory, leaving every other path untouched. It's the CLI-local +// name for the shared pathutil.ExpandHome; cluster.expandPath resolves +// to the same helper, so ~-expansion is identical across subcommands +// (a --kubeconfig ~alice/... resolves alice's home just like a data +// ingest path does). See pathutil.ExpandHome for the full contract. (#181) func expandHome(path string) string { - if path == "" || path[0] != '~' { - return path - } - home, err := os.UserHomeDir() - if err != nil { - // Can't resolve $HOME — leave it and let the downstream - // Discover* error mention the literal path, which is more - // useful than a generic failure here. - return path + return pathutil.ExpandHome(path) +} + +// statDatasetPath is the "path existence FIRST" guard (#181): a typo'd +// path fails plainly on the path — a clean "no such file or directory" — +// before any family sniff, label preview, or schema work touches it. +// Both entry points call it: the flag-only path from runDataIngest's 0b +// step, and the guided path from runInteractive (before the family sniff), +// so the invariant holds on every route rather than only the flag path. +func statDatasetPath(path string) error { + if _, serr := os.Stat(path); serr != nil { + if errors.Is(serr, os.ErrNotExist) { + return &exitError{code: 3, err: fmt.Errorf( + "no such file or directory: %q — check the path to your dataset", path)} + } + return &exitError{code: 3, err: fmt.Errorf( + "can't read %q: %w", path, serr)} } - // path[1:] is "" for "~" (→ home) and "/x" for "~/x" (→ home/x); - // filepath.Join cleans the join either way. - return filepath.Join(home, path[1:]) + return nil } // runDataIngest is the full Phase 3 implementation: pre-flight @@ -437,6 +472,14 @@ collaborators can train against that table without ever seeing the raw files.`)) a.Printer.Infof("Cancelled — nothing was ingested.") return nil } + // A typed exitError from a guided step (e.g. the path-existence + // guard, which runInteractive runs before the family sniff) + // already carries its own code + clean message — surface it as-is + // rather than burying it under "interactive setup:". + var ee *exitError + if errors.As(err, &ee) { + return err + } return &exitError{code: 3, err: fmt.Errorf("interactive setup: %w", err)} } } @@ -460,6 +503,20 @@ collaborators can train against that table without ever seeing the raw files.`)) // before any push.Discover* call. (#37) a.LocalPath = expandHome(a.LocalPath) + // 0b. Path existence FIRST — before any spec / schema / family + // validation. A typo'd path should fail on the path with a plain + // "no such file or directory", not surface later as a confusing + // downstream error (e.g. the task gate asking which task the + // non-existent data is for). runInteractive runs this same guard + // before its family sniff / label preview, so the invariant holds on + // the guided route too; this re-check covers the flag-only path and + // is cheap (one stat). The family walk below stats again for its + // layout-specific diagnostics; this is only about ordering the first + // failure a customer sees. (#181) + if err := statDatasetPath(a.LocalPath); err != nil { + return err + } + // 1. Validate the table name BEFORE anything else. It's both // the MySQL identifier and the /data/shared// PVC // subdirectory — an unsanitized traversal name (../../etc) diff --git a/internal/cli/data_test.go b/internal/cli/data_test.go index 2dfad932..387ab464 100644 --- a/internal/cli/data_test.go +++ b/internal/cli/data_test.go @@ -212,6 +212,85 @@ func TestDataIngest_NonexistentLocalPath_ExitsThree(t *testing.T) { } } +// TestDataIngest_NonexistentPath_BeatsTaskGate: a typo'd path must fail on +// the path (exit 3), NOT on a downstream spec/family error, even when the +// task is also wrong. Pins the #181 ordering fix: path existence is checked +// before the category gate (which would otherwise exit 2 for the bad task +// and send the user chasing the wrong problem). +func TestDataIngest_NonexistentPath_BeatsTaskGate(t *testing.T) { + code, _, _ := execDataIngest(t, []string{ + "/tmp/tracebloc-cli-test-no-such-dir-" + t.Name(), + "--name=t1", + "--task=definitely-not-a-task", // would be exit 2 at the task gate + "--intent=train", + }) + if code != 3 { + t.Fatalf("expected exit 3 (path checked before the task gate), got %d", code) + } +} + +// TestDataIngest_BareCSVFile_Accepted: a bare .csv is a valid tabular input +// (#181). It gets PAST the layout walk — proven by the "Inferred schema" +// line, which prints only after DiscoverTabular accepted the file — and then +// falls through the local checks to the injected bad kubeconfig (exit 3), +// the same fall-through a valid directory reaches. +func TestDataIngest_BareCSVFile_Accepted(t *testing.T) { + dir := t.TempDir() + csv := filepath.Join(dir, "churn.csv") + if err := os.WriteFile(csv, []byte("age,churned\n30,yes\n40,no\n"), 0o644); err != nil { + t.Fatalf("write csv: %v", err) + } + code, stdout, _ := execDataIngest(t, []string{ + csv, + "--name=churn", + "--task=tabular_classification", + "--intent=train", + "--label-column=churned", + }) + if code != 3 { + t.Fatalf("expected exit 3 (bare .csv accepted, then bad kubeconfig), got %d", code) + } + if !strings.Contains(stdout, "Inferred schema") { + t.Errorf("want the schema-inference line proving the bare .csv passed the walk; stdout:\n%s", stdout) + } +} + +// TestDataIngest_ImageBareFile_ExitsThree: the image family is directory-only. +// A bare .csv passed as image_classification is rejected at the walk (exit 3) +// and never reaches schema inference (that's tabular-only). +func TestDataIngest_ImageBareFile_ExitsThree(t *testing.T) { + dir := t.TempDir() + csv := filepath.Join(dir, "labels.csv") + if err := os.WriteFile(csv, []byte("image_id,label\n1.jpg,c\n"), 0o644); err != nil { + t.Fatalf("write csv: %v", err) + } + code, stdout, _ := execDataIngest(t, []string{ + csv, + "--name=imgs", + "--task=image_classification", + "--intent=train", + "--label-column=label", + }) + if code != 3 { + t.Fatalf("expected exit 3 for a bare file passed as image, got %d", code) + } + if strings.Contains(stdout, "Inferred schema") { + t.Errorf("image walk must not run tabular schema inference on a bare file; stdout:\n%s", stdout) + } +} + +// TestDataIngestCmd_UsesDatasetArgName: the positional arg is +// (renamed from , #181) in the command's Use string and help. +func TestDataIngestCmd_UsesDatasetArgName(t *testing.T) { + cmd := newDataIngestCmd() + if !strings.Contains(cmd.Use, "") { + t.Errorf("Use = %q, want it to name the arg ", cmd.Use) + } + if strings.Contains(cmd.Use, "") { + t.Errorf("Use = %q still uses the old name", cmd.Use) + } +} + // TestDataIngest_MissingLabelsCSV_ExitsThree: most likely "real // world" wrong-layout case — customer has images but forgot // labels.csv. Pins the exit-code contract for the common failure diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index 5f28b277..44ab07b9 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -136,8 +136,8 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo // (c) path — then detect the family from the layout and echo it back. if a.LocalPath == "" { - p.PromptHint("The folder holding your data — a single .csv for a table, or labels.csv + an images/ folder for images. e.g. ~/datasets/churn") - ans, err := pr.Input("Where is your data? (the folder holding it)", "e.g. ./my-data", "", validateDatasetPath) + p.PromptHint("The file or folder holding your data — a single .csv for a table, or labels.csv + an images/ folder for images. e.g. ~/datasets/churn") + ans, err := pr.Input("Where is your data? (file or folder)", "e.g. ./my-data", "", validateDatasetPath) if err != nil { return err } @@ -153,6 +153,16 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo // the real path; runDataIngest's own expandHome then no-ops. a.LocalPath = expandHome(a.LocalPath) + // Path existence FIRST (#181): fail plainly on a typo'd path here, before + // the family sniff / label preview below touch it — otherwise the user + // answers the whole questionnaire (family, task, label) against a path + // that doesn't exist, only to hit the hard error afterward. runDataIngest + // re-checks for the flag-only route; this keeps the invariant on the + // guided route too. The exitError propagates unwrapped (see runDataIngest). + if err := statDatasetPath(a.LocalPath); err != nil { + 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, diff --git a/internal/cli/interactive_test.go b/internal/cli/interactive_test.go index 529ed740..c15e35a0 100644 --- a/internal/cli/interactive_test.go +++ b/internal/cli/interactive_test.go @@ -103,11 +103,11 @@ func textDirLayout(t *testing.T) string { func TestRunInteractive_PromptOrder(t *testing.T) { dir := tabularDir(t) f := &fakePrompter{answers: map[string]string{ - "Is this training or test data?": "test", - "What should we call this dataset?": "churn_train", - "Where is your data? (the folder holding it)": dir, - "Which task?": "Tabular classification", - "Which column holds the class?": "churned", + "Is this training or test data?": "test", + "What should we call this dataset?": "churn_train", + "Where is your data? (file or folder)": dir, + "Which task?": "Tabular classification", + "Which column holds the class?": "churned", }} a := &runDataIngestArgs{} if err := runInteractive(discardPrinter(), f, a, false /*taskSet*/); err != nil { @@ -119,7 +119,7 @@ func TestRunInteractive_PromptOrder(t *testing.T) { want := []string{ "Is this training or test data?", "What should we call this dataset?", - "Where is your data? (the folder holding it)", + "Where is your data? (file or folder)", "Which task?", "Which column holds the class?", } @@ -133,6 +133,36 @@ func TestRunInteractive_PromptOrder(t *testing.T) { } } +// TestRunInteractive_PathPromptCopyIsFileOrFolder pins the #181 copy +// restoration: now that the walk accepts a bare .csv, the path prompt says +// "file or folder" again (softened to folder-only in #180b). +func TestRunInteractive_PathPromptCopyIsFileOrFolder(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Is this training or test data?": "train", + "What should we call this dataset?": "churn", + "Where is your data? (file or folder)": dir, + "Which task?": "Tabular classification", + "Which column holds the class?": "churned", + }} + a := &runDataIngestArgs{} + if err := runInteractive(discardPrinter(), f, a, false /*taskSet*/); err != nil { + t.Fatalf("runInteractive: %v", err) + } + found := false + for _, label := range f.asked { + if label == "Where is your data? (file or folder)" { + found = true + } + if strings.Contains(label, "the folder holding it") { + t.Errorf("path prompt still uses the folder-only copy: %q", label) + } + } + if !found { + t.Errorf("path prompt label not asked; got %v", f.asked) + } +} + // TestRunInteractive_SniffEchoesFamily: a confident layout is echoed back // and the family question is NOT asked (the sniff is enough). func TestRunInteractive_SniffEchoesFamily(t *testing.T) { @@ -452,8 +482,8 @@ func TestRunInteractive_RejectsBadName(t *testing.T) { // directory (empty path → Abs("") → cwd). func TestRunInteractive_RejectsEmptyPath(t *testing.T) { f := &fakePrompter{answers: map[string]string{ - "What should we call this dataset?": "t", - "Where is your data? (the folder holding it)": " ", + "What should we call this dataset?": "t", + "Where is your data? (file or folder)": " ", }} a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} if err := runInteractive(discardPrinter(), f, a, false); err == nil { @@ -469,9 +499,9 @@ func TestRunInteractive_RejectsEmptyPath(t *testing.T) { 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", + "What should we call this dataset?": "t", + "Where is your data? (file or folder)": " " + dir + " ", + "Which column holds the class?": "churned", }} a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} if err := runInteractive(discardPrinter(), f, a, false); err != nil { diff --git a/internal/cluster/kubeconfig.go b/internal/cluster/kubeconfig.go index 2a943806..4185a232 100644 --- a/internal/cluster/kubeconfig.go +++ b/internal/cluster/kubeconfig.go @@ -12,12 +12,12 @@ package cluster import ( "fmt" - "os" - "path/filepath" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" + + "github.com/tracebloc/cli/internal/pathutil" ) // KubeconfigOptions captures every knob the customer can turn when @@ -142,23 +142,13 @@ func NewClientset(rc *ResolvedConfig) (kubernetes.Interface, error) { } // expandPath handles `~/.kube/config` → `/home/user/.kube/config` -// since clientcmd's ExplicitPath wants an absolute path. Empty -// strings pass through unchanged (they signal "use defaults" to -// clientcmd). +// (and `~alice/.kube/config` → alice's home) since clientcmd's +// ExplicitPath wants an absolute path. Empty strings pass through +// unchanged (they signal "use defaults" to clientcmd). It delegates to +// the shared pathutil.ExpandHome so ~-expansion is identical to the +// data-ingest path helper — same ~user syntax, same result, every +// subcommand. An unresolvable home is returned unexpanded so clientcmd's +// own "tried to read ~/.kube/config and got X" error still surfaces. func expandPath(p string) string { - if p == "" { - return "" - } - if p[0] != '~' { - return p - } - home, err := os.UserHomeDir() - if err != nil { - // Best-effort: return the unexpanded path and let - // clientcmd's error surface mention it. Failing here would - // hide the more useful "tried to read ~/.kube/config and - // got X" error downstream. - return p - } - return filepath.Join(home, p[1:]) + return pathutil.ExpandHome(p) } diff --git a/internal/pathutil/expand.go b/internal/pathutil/expand.go new file mode 100644 index 00000000..5ccb42a5 --- /dev/null +++ b/internal/pathutil/expand.go @@ -0,0 +1,55 @@ +// Package pathutil holds small filesystem-path helpers shared across +// the CLI. It's a leaf package (no internal imports) so both +// internal/cli and internal/cluster can use it without importing each +// other — the consolidation the two former copies of this expander +// kept promising in their doc comments. +package pathutil + +import ( + "os" + "os/user" + "path/filepath" + "strings" +) + +// ExpandHome expands a leading ~ to a home directory, leaving every +// other path (relative, absolute, empty) untouched: +// +// - "" → "" (callers read empty as "use defaults") +// - "~" and "~/…" → the current user's $HOME +// - "~user" and "~user/…" → that named user's home (via user.Lookup) +// +// When a home can't be resolved the literal path is returned unchanged, +// so the caller's own path-existence check reports it plainly ("no such +// file or directory: ~bob/data") instead of us silently mangling it into +// a relative path. That covers three cases: +// +// - no $HOME for the current user, +// - an unknown or unlookupable ~user (a static CGO-less binary can't +// read /etc/passwd for a foreign user), and +// - a resolvable account whose passwd home-directory field is blank — +// user.Lookup succeeds with an empty HomeDir, and joining "" would +// yield a relative path, so we treat it like a resolution failure. +func ExpandHome(path string) string { + if path == "" || path[0] != '~' { + return path + } + // "~" or "~/…" → the current user's home. path[1:] is "" for "~" + // (→ home) and "/x" for "~/x" (→ home/x); filepath.Join cleans it. + if len(path) == 1 || path[1] == '/' { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return path + } + return filepath.Join(home, path[1:]) + } + // "~user" or "~user/…" → the named user's home. Split the username + // off at the first slash; the remainder (possibly empty) joins onto + // their home directory. + name, rest, _ := strings.Cut(path[1:], "/") + u, err := user.Lookup(name) + if err != nil || u.HomeDir == "" { + return path + } + return filepath.Join(u.HomeDir, rest) +} diff --git a/internal/pathutil/expand_test.go b/internal/pathutil/expand_test.go new file mode 100644 index 00000000..793f4c80 --- /dev/null +++ b/internal/pathutil/expand_test.go @@ -0,0 +1,69 @@ +package pathutil + +import ( + "os" + "os/user" + "path/filepath" + "testing" +) + +// TestExpandHome_Basics pins the non-lookup contract: empty, relative, +// absolute, and non-tilde paths pass through untouched; "~" and "~/…" +// resolve under the current user's $HOME. +func TestExpandHome_Basics(t *testing.T) { + home, err := os.UserHomeDir() + if err != nil || home == "" { + t.Skipf("no home dir: %v", err) + } + cases := []struct{ in, want string }{ + {"", ""}, + {"relative/path", "relative/path"}, + {"/absolute/path", "/absolute/path"}, + {"./x", "./x"}, + {"~", home}, + {"~/", home}, + {"~/x", filepath.Join(home, "x")}, + {"~/a/b", filepath.Join(home, "a", "b")}, + } + for _, c := range cases { + if got := ExpandHome(c.in); got != c.want { + t.Errorf("ExpandHome(%q) = %q, want %q", c.in, got, c.want) + } + } +} + +// TestExpandHome_NamedUser covers the ~user form (#181): "~user" and +// "~user/…" resolve under that user's home. We look up the CURRENT user by +// name so the test doesn't depend on a fixed account, and an unknown ~user +// is left literal so the caller's path-existence check surfaces it. +func TestExpandHome_NamedUser(t *testing.T) { + u, err := user.Current() + if err != nil || u.Username == "" { + t.Skipf("no current user: %v", err) + } + looked, err := user.Lookup(u.Username) + if err != nil { + t.Skipf("user.Lookup(%q) unsupported here: %v", u.Username, err) + } + if looked.HomeDir == "" { + t.Skipf("current user has a blank home dir") + } + home := looked.HomeDir + + cases := []struct{ in, want string }{ + {"~" + u.Username, home}, + {"~" + u.Username + "/data", filepath.Join(home, "data")}, + {"~" + u.Username + "/a/b", filepath.Join(home, "a", "b")}, + } + for _, c := range cases { + if got := ExpandHome(c.in); got != c.want { + t.Errorf("ExpandHome(%q) = %q, want %q", c.in, got, c.want) + } + } + + // An unknown user can't be resolved: the literal is returned unchanged. + const unknown = "~nsuchuser-tracebloc-181/x" + if got := ExpandHome(unknown); got != unknown { + t.Errorf("ExpandHome(%q) = %q, want it left literal", unknown, got) + } +} diff --git a/internal/push/preview.go b/internal/push/preview.go index 39a9da5a..f1248524 100644 --- a/internal/push/preview.go +++ b/internal/push/preview.go @@ -30,9 +30,9 @@ type FamilySniff struct { // SniffFamily previews the family of the dataset at path by looking for // the same layout markers Discover / DiscoverText / DiscoverTabular key // on — labels.csv + an images/ dir (image), labels.csv + a texts/ or -// sequences/ dir (text), or exactly one CSV in a directory with none of -// those (tabular). It reads directory entries only; it opens no files and -// validates nothing. +// sequences/ dir (text), exactly one CSV in a directory with none of +// those (tabular), or a bare .csv file (tabular). It reads directory +// entries only; it opens no files and validates nothing. // // It never claims more than the matching Discover* would accept: the // marker directories (images/, texts/, sequences/) and labels.csv are @@ -40,18 +40,20 @@ type FamilySniff struct { // Lstats — a mis-cased "Images/" is not the walk's marker, so it is not // sniffed as confident image. Image / text are confident only when BOTH // labels.csv AND the subdir are present, mirroring Discover / DiscoverText. -// Tabular is confident only on EXACTLY ONE CSV, mirroring DiscoverTabular's -// findSingleCSV count rule — a directory with two or more CSVs is a layout -// the tabular walk refuses, so the sniff must not confidently place it -// either. Only the .csv extension match stays case-insensitive, mirroring +// Tabular is confident on EXACTLY ONE CSV in a directory, mirroring +// DiscoverTabular's findSingleCSV count rule — a directory with two or more +// CSVs is a layout the tabular walk refuses, so the sniff must not +// confidently place it either — OR on a bare .csv file, which +// DiscoverTabular now stages as the one CSV under the dataset (#181). Only +// the .csv extension match stays case-insensitive, mirroring // DiscoverTabular's EqualFold. // -// Every family's walk requires a directory (bare-file support is -// cli#181), so a file path is never a confident sniff. Anything we can't -// place — a missing path, a bare file, a directory with no recognizable -// marker, an image+text mix, an image/text dir without labels.csv, a -// multi-CSV directory the tabular walk would reject — comes back -// Confident=false so the caller asks the family plainly. +// The media/label families (image, text) require a directory, so a bare +// file that is not a .csv is never a confident sniff. Anything we can't +// place — a missing path, a non-.csv bare file, a directory with no +// recognizable marker, an image+text mix, an image/text dir without +// labels.csv, a multi-CSV directory the tabular walk would reject — comes +// back Confident=false so the caller asks the family plainly. func SniffFamily(path string) FamilySniff { abs, err := filepath.Abs(path) if err != nil { @@ -64,11 +66,22 @@ func SniffFamily(path string) FamilySniff { return FamilySniff{} } - // Every family's walk requires a directory; a bare file (even a .csv) - // is rejected by DiscoverTabular until cli#181 adds bare-file support. - // Stay ambiguous so the caller asks the family plainly rather than - // promising a layout the walk would refuse. + // A bare file: only a .csv is placeable (tabular), mirroring the + // bare-file shape DiscoverTabular accepts (#181). The image / text + // families need a directory, so any other bare file stays ambiguous. + // + // Lstat first: os.Stat above followed a symlink, but DiscoverTabular + // stats the CSV with Lstat and rejectSymlinks it — so a symlinked .csv + // is a layout the walk REFUSES. Sniffing it as confident tabular would + // break this func's "never claims more than the matching Discover* would + // accept" contract (it'd lock the guided flow to tabular, then hard-fail + // on the symlink guard). Treat a symlink like any other unplaceable file. if !st.IsDir() { + if li, lerr := os.Lstat(abs); lerr == nil && + li.Mode()&os.ModeSymlink == 0 && isCSV(abs) { + return FamilySniff{Family: FamilyTabular, Confident: true, + Echo: "Found a CSV table — this is tabular data."} + } return FamilySniff{} } @@ -110,7 +123,7 @@ func SniffFamily(path string) FamilySniff { if name == "labels.csv" { hasLabels = true } - if strings.EqualFold(filepath.Ext(name), ".csv") { + if isCSV(name) { csvCount++ } } diff --git a/internal/push/preview_test.go b/internal/push/preview_test.go index 17b72f4f..dba65827 100644 --- a/internal/push/preview_test.go +++ b/internal/push/preview_test.go @@ -44,15 +44,50 @@ func TestSniffFamily(t *testing.T) { } }) - t.Run("bare .csv file is ambiguous (walk requires a directory)", func(t *testing.T) { - // DiscoverTabular rejects a bare file (bare-file support is cli#181), - // so the sniff must not confidently place a lone .csv — otherwise it - // promises a layout the walk refuses. + t.Run("bare .csv file is confident tabular (walk now accepts it)", func(t *testing.T) { + // DiscoverTabular now stages a bare .csv as the one CSV under the + // dataset (cli#181), so the sniff confidently places a lone .csv as + // tabular — mirroring the shape the walk accepts. dir := t.TempDir() csv := filepath.Join(dir, "t.csv") writePrev(t, csv, "a,b\n1,2\n") - if s := SniffFamily(csv); s.Confident { - t.Fatalf("a bare .csv file should be ambiguous, got %+v", s) + s := SniffFamily(csv) + if !s.Confident || s.Family != FamilyTabular { + t.Fatalf("a bare .csv file should sniff confident tabular, got %+v", s) + } + // And the walk it mirrors accepts the same bare file. + if _, err := DiscoverTabular(csv); err != nil { + t.Fatalf("DiscoverTabular should accept a bare .csv: %v", err) + } + }) + + t.Run("bare non-.csv file is ambiguous (media families need a folder)", func(t *testing.T) { + dir := t.TempDir() + txt := filepath.Join(dir, "notes.txt") + writePrev(t, txt, "hello") + if s := SniffFamily(txt); s.Confident { + t.Fatalf("a bare non-.csv file should be ambiguous, got %+v", s) + } + }) + + t.Run("symlinked .csv is ambiguous, matching the walk's symlink rejection", func(t *testing.T) { + // DiscoverTabular rejects a symlinked CSV (rejectSymlink), so the + // sniff must not confidently promise tabular for one — otherwise the + // guided flow locks to tabular, then hard-fails on the walk. Sniff and + // walk must agree: both refuse. (cli#202 review) + dir := t.TempDir() + real := filepath.Join(dir, "real.csv") + writePrev(t, real, "a,b\n1,2\n") + link := filepath.Join(dir, "link.csv") + if err := os.Symlink(real, link); err != nil { + t.Skipf("symlink unsupported on this platform: %v", err) + } + if s := SniffFamily(link); s.Confident { + t.Fatalf("a symlinked .csv should be ambiguous (walk rejects it), got %+v", s) + } + // And the walk it mirrors does reject the same symlinked file. + if _, err := DiscoverTabular(link); err == nil { + t.Fatalf("DiscoverTabular should reject a symlinked .csv") } }) diff --git a/internal/push/tabular.go b/internal/push/tabular.go index ecc14589..ac0256b2 100644 --- a/internal/push/tabular.go +++ b/internal/push/tabular.go @@ -39,15 +39,30 @@ var reservedColumns = map[string]bool{ // turns float on row 10k) is the case --schema exists to override. const schemaInferenceSampleRows = 5000 -// DiscoverTabular validates a local directory for a tabular / -// time-series ingestion. Unlike the image layout, tabular categories -// have NO sidecar files — the dataset IS a single CSV. The directory -// must contain exactly one .csv file; that becomes the labels/data -// CSV staged for the ingestor. +// DiscoverTabular validates a local input for a tabular / time-series +// ingestion. Unlike the image layout, tabular categories have NO +// sidecar files — the dataset IS a single CSV. Two shapes are accepted +// (#181): +// +// - a bare .csv file: the dataset itself, passed directly; +// - a directory containing exactly one .csv file. +// +// Both resolve to the SAME staged layout — the CSV is staged as the one +// labels.csv under the dataset — so the ingestor's contract is unchanged +// (this is a CLI-side input convenience, not an ingestor-side change). // // The returned LocalLayout reuses the image layout's LabelsCSV field // (staged as labels.csv) with an empty Images slice, so the existing // tar/stream machinery handles it unchanged. +// isCSV reports whether name has a .csv extension, matched +// case-insensitively. It's the single rule DiscoverTabular's walk, its +// bare-file branch, and SniffFamily all key on — shared so the sniff's +// "confident tabular" promise can never drift from what the walk actually +// accepts (the exact lockstep the surrounding comments rely on). +func isCSV(name string) bool { + return strings.EqualFold(filepath.Ext(name), ".csv") +} + // findSingleCSV resolves the one .csv file a tabular layout must hold in // dir, enforcing DiscoverTabular's exactly-one rule: zero or multiple CSVs // are errors with the same framing. dir must already be known to be a @@ -64,7 +79,7 @@ func findSingleCSV(dir string) (string, error) { if e.IsDir() { continue } - if strings.EqualFold(filepath.Ext(e.Name()), ".csv") { + if isCSV(e.Name()) { csvs = append(csvs, e.Name()) } } @@ -92,16 +107,31 @@ func DiscoverTabular(rootDir string) (*LocalLayout, error) { } st, err := os.Stat(abs) if err != nil { - return nil, fmt.Errorf("reading dataset directory %q: %w", abs, err) - } - if !st.IsDir() { - return nil, fmt.Errorf( - "%q is not a directory; pass the directory containing the dataset CSV", abs) + return nil, fmt.Errorf("reading dataset path %q: %w", abs, err) } - csvPath, err := findSingleCSV(abs) - if err != nil { - return nil, err + // Resolve the CSV + the layout root from either shape. A directory + // takes DiscoverTabular's exactly-one-CSV rule (findSingleCSV); a bare + // file is accepted only when it's a .csv — the dataset IS that CSV, so + // it stages identically to a one-CSV directory (#181). The root is the + // directory either way (the file's parent for the bare-file case), so + // the pre-flight summary's "root" field stays a directory. + var csvPath, root string + if st.IsDir() { + root = abs + csvPath, err = findSingleCSV(abs) + if err != nil { + return nil, err + } + } else { + if !isCSV(abs) { + return nil, fmt.Errorf( + "%q is not a .csv file. Tabular / time-series data is a single CSV — "+ + "pass the .csv file itself, or a directory containing exactly one .csv.", + abs) + } + root = filepath.Dir(abs) + csvPath = abs } csvName := filepath.Base(csvPath) // Lstat (not Stat) so a symlinked CSV is rejected rather than @@ -117,7 +147,7 @@ func DiscoverTabular(rootDir string) (*LocalLayout, error) { return nil, sizeError(csvName, info.Size(), MaxSingleFileBytes) } - layout := &LocalLayout{Root: abs, LabelsCSV: csvPath, TotalBytes: info.Size()} + layout := &LocalLayout{Root: root, LabelsCSV: csvPath, TotalBytes: info.Size()} if layout.TotalBytes > MaxTotalBytes { return nil, fmt.Errorf( "dataset is %s, exceeds v0.1 cap of %s. For larger datasets, the "+ diff --git a/internal/push/tabular_test.go b/internal/push/tabular_test.go index 28c6d026..d47d920e 100644 --- a/internal/push/tabular_test.go +++ b/internal/push/tabular_test.go @@ -1,6 +1,8 @@ package push import ( + "archive/tar" + "bytes" "os" "path/filepath" "testing" @@ -38,6 +40,86 @@ func TestDiscoverTabular_SingleCSV(t *testing.T) { } } +// TestDiscoverTabular_BareCSVFile: a bare .csv file (not a directory) is +// accepted for tabular (#181). It resolves to a layout whose LabelsCSV is +// that file, Root is the file's parent directory, and Images is empty — the +// SAME shape a single-CSV directory produces, so the tar/stream machinery +// stages it identically (as labels.csv under the dataset). +func TestDiscoverTabular_BareCSVFile(t *testing.T) { + dir := t.TempDir() + csv := writeFile(t, dir, "churn.csv", "age,churned\n30,yes\n40,no\n") + + layout, err := DiscoverTabular(csv) + if err != nil { + t.Fatalf("DiscoverTabular(bare .csv): %v", err) + } + if layout.LabelsCSV != csv { + t.Errorf("LabelsCSV = %q, want %q", layout.LabelsCSV, csv) + } + if layout.Root != dir { + t.Errorf("Root = %q, want the file's parent dir %q", layout.Root, dir) + } + if len(layout.Images) != 0 { + t.Errorf("Images = %v, want empty", layout.Images) + } + if layout.TotalBytes == 0 { + t.Errorf("TotalBytes = 0, want the CSV's size") + } +} + +// TestDiscoverTabular_BareFileVsDirSameStaging: a bare .csv and a directory +// holding that same CSV must stage byte-for-identically — both land the CSV +// as labels.csv at the dataset root — so bare-file support is a pure CLI-side +// input convenience and never changes what the ingestor reads. +func TestDiscoverTabular_BareFileVsDirSameStaging(t *testing.T) { + body := "age,churned\n30,yes\n40,no\n" + fileDir := t.TempDir() + bare := writeFile(t, fileDir, "churn.csv", body) + // Directory holding the same single CSV. + someDir := t.TempDir() + writeFile(t, someDir, "churn.csv", body) + + fileL, err := DiscoverTabular(bare) + if err != nil { + t.Fatalf("DiscoverTabular(file): %v", err) + } + dirL, err := DiscoverTabular(someDir) + if err != nil { + t.Fatalf("DiscoverTabular(dir): %v", err) + } + + var fileTar, dirTar bytes.Buffer + if err := writeLayoutTar(&fileTar, fileL); err != nil { + t.Fatalf("writeLayoutTar(file): %v", err) + } + if err := writeLayoutTar(&dirTar, dirL); err != nil { + t.Fatalf("writeLayoutTar(dir): %v", err) + } + if !bytes.Equal(fileTar.Bytes(), dirTar.Bytes()) { + t.Error("bare-file and single-CSV-dir produced different staged tars; they must be identical") + } + // And the one entry is labels.csv. + tr := tar.NewReader(&fileTar) + hdr, err := tr.Next() + if err != nil { + t.Fatalf("reading tar entry: %v", err) + } + if hdr.Name != "labels.csv" { + t.Errorf("staged entry = %q, want labels.csv", hdr.Name) + } +} + +// TestDiscoverTabular_BareNonCSVFile: a bare file that isn't a .csv is a +// clear error — tabular data is a single CSV, so we say so rather than +// letting a downstream reader choke. +func TestDiscoverTabular_BareNonCSVFile(t *testing.T) { + dir := t.TempDir() + txt := writeFile(t, dir, "notes.txt", "hello") + if _, err := DiscoverTabular(txt); err == nil { + t.Error("DiscoverTabular(bare .txt) returned nil error, want a clear .csv-required error") + } +} + // TestDiscoverTabular_NoCSV and _MultipleCSV: the layout requires // exactly one CSV; zero or many is a clear, actionable error rather // than a guess. diff --git a/internal/push/text_test.go b/internal/push/text_test.go index 472bb450..9556ce62 100644 --- a/internal/push/text_test.go +++ b/internal/push/text_test.go @@ -3,6 +3,7 @@ package push import ( "os" "path/filepath" + "strings" "testing" "gopkg.in/yaml.v3" @@ -34,6 +35,21 @@ func mkTextDir(t *testing.T, sidecar string, withTokenizer bool) string { return dir } +// TestDiscoverText_BareFileRejected: the text layout is directory-only. +// A bare file (even a .csv) must be rejected with a clear "not a directory" +// error — bare-file support is tabular-only (#181). +func TestDiscoverText_BareFileRejected(t *testing.T) { + dir := t.TempDir() + bare := writeFile(t, dir, "labels.csv", "filename,label\na.txt,pos\n") + _, err := DiscoverText("text_classification", bare) + if err == nil { + t.Fatal("DiscoverText(bare file) returned nil error; text layout must require a directory") + } + if !strings.Contains(err.Error(), "not a directory") { + t.Errorf("error = %q, want it to say the path is not a directory", err) + } +} + // TestDiscoverText_Classification: text_classification stages // labels.csv + the texts/ directory, no images, no extra files. func TestDiscoverText_Classification(t *testing.T) { diff --git a/internal/push/walk_test.go b/internal/push/walk_test.go index 114cbe8f..e77cf8e2 100644 --- a/internal/push/walk_test.go +++ b/internal/push/walk_test.go @@ -113,6 +113,24 @@ func TestDiscover_SkipsNonImageFiles(t *testing.T) { } } +// TestDiscover_BareFileRejected: the image layout is directory-only. A bare +// file (even a .csv) must be rejected with a clear "not a directory" error — +// bare-file support is tabular-only (#181), so image datasets can't shortcut it. +func TestDiscover_BareFileRejected(t *testing.T) { + dir := t.TempDir() + bare := filepath.Join(dir, "labels.csv") + if err := os.WriteFile(bare, []byte("image_id,label\n1.jpg,c\n"), 0o644); err != nil { + t.Fatal(err) + } + _, err := Discover(bare) + if err == nil { + t.Fatal("Discover(bare file) returned nil error; image layout must require a directory") + } + if !strings.Contains(err.Error(), "not a directory") { + t.Errorf("error = %q, want it to say the path is not a directory", err) + } +} + func TestDiscover_MissingLabelsCSV(t *testing.T) { root := t.TempDir() if err := os.MkdirAll(filepath.Join(root, "images"), 0o755); err != nil {