diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 39d13bb1b4..d2f582b656 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -451,10 +451,26 @@ func (p *Pipeline) provision(ctx context.Context, pyMinor string) error { // populates the venv path. dbcPin is "" in constraints-only mode, where the DB // Connect assertion is skipped. func (p *Pipeline) validate(ctx context.Context, expectedPyMinor, dbcPin string) error { - pyVer, dbcVer, err := p.PM.Validate(ctx, p.ProjectDir) + info, err := p.PM.Validate(ctx, p.ProjectDir) if err != nil { return p.fail(PhaseValidate, true, asPipelineError(err, ErrValidate, "validation failed")) } + pyVer, dbcVer := info.PythonMinor, info.DBConnect + + // A standalone pyspark installed alongside databricks-connect collides only when the + // collision is *live*. databricks-connect vendors its own pyspark, so the two share + // the pyspark namespace and whichever install's files win the overwrite decide + // whether `import databricks.connect` works. When it does not, the environment + // genuinely cannot start a session (surfacing to users as an opaque Java or protobuf + // error), so fail here rather than report it ready. But a stale, orphaned pyspark + // dist-info left behind by an install databricks-connect's files won leaves the + // metadata probe reporting a pyspark version while the environment imports fine — + // failing on that would reject a working setup, so require an actual import failure. + if dbcVer != "" && info.Pyspark != "" && info.DBConnectImportErr != "" { + return p.fail(PhaseValidate, true, NewError(ErrValidate, nil, + "databricks-connect %s cannot be imported (%s) because a standalone pyspark %s is installed alongside it — they share the pyspark package and overwrite each other. Remove the standalone pyspark dependency from your project and re-run setup; if you need a local Spark session, keep it in a separate virtual environment", + dbcVer, info.DBConnectImportErr, info.Pyspark)) + } // Assert the installed Python minor matches the target. if pyVer != expectedPyMinor { diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index c4748a3198..b50291d833 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -27,15 +27,15 @@ const cancelPMStderr = "error: no solution found: databricks-connect==17.2 confl // below — a plain ">= 0" assertion would also hold for the unset field. const fetchDelay = 25 * time.Millisecond -type fakePM struct{ py, dbc string } +type fakePM struct{ py, dbc, pyspark, dbcImportErr string } func (fakePM) Name() string { return "fake" } func (fakePM) EnsureAvailable(context.Context) (string, error) { return "fake 1.0", nil } func (fakePM) EnsurePython(context.Context, string) error { return nil } func (fakePM) Provision(context.Context, string, string) error { return nil } func (fakePM) PostProvision(context.Context, string) error { return nil } -func (f fakePM) Validate(context.Context, string) (string, string, error) { - return f.py, f.dbc, nil +func (f fakePM) Validate(context.Context, string) (VenvInfo, error) { + return VenvInfo{PythonMinor: f.py, DBConnect: f.dbc, Pyspark: f.pyspark, DBConnectImportErr: f.dbcImportErr}, nil } // noProvisionPM fails any method that could touch the machine (install the @@ -60,8 +60,8 @@ func (noProvisionPM) PostProvision(context.Context, string) error { return errors.New("PostProvision must not be called under --dry-run") } -func (noProvisionPM) Validate(context.Context, string) (string, string, error) { - return "", "", errors.New("Validate must not be called under --dry-run") +func (noProvisionPM) Validate(context.Context, string) (VenvInfo, error) { + return VenvInfo{}, errors.New("Validate must not be called under --dry-run") } // uvMissingPM fails EnsureAvailable, simulating a machine where the package @@ -881,6 +881,52 @@ func TestPipelineValidateRejectsUnparseablePin(t *testing.T) { assert.Equal(t, PhaseValidate, res.Error.FailurePhase) } +func TestPipelineValidateRejectsStandalonePyspark(t *testing.T) { + // A LIVE collision: standalone pyspark installed alongside databricks-connect, and + // `import databricks.connect` fails as a result. The environment cannot start a + // session, so validate must fail with actionable guidance rather than report ready. + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0", pyspark: "4.2.0", dbcImportErr: "ImportError"}, + } + res, err := p.Run(t.Context()) + require.Error(t, err) + require.NotNil(t, res.Error) + assert.Equal(t, ErrValidate, res.Error.Code) + assert.Equal(t, PhaseValidate, res.Error.FailurePhase) + assert.Contains(t, res.Error.Msg, "pyspark") + assert.Contains(t, res.Error.Msg, "databricks-connect") + assert.Contains(t, res.Error.Msg, "ImportError") +} + +func TestPipelineValidateAllowsStalePysparkDistInfo(t *testing.T) { + // A standalone pyspark distribution is present in the metadata, but databricks-connect + // imports fine — its vendored files won the overwrite, leaving only an orphaned + // pyspark dist-info. The environment is functional, so validate must NOT hard-fail + // on the mere presence of pyspark metadata (regression guard for the false positive + // reported in review: install-order can leave a working env with a stale dist-info). + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + // pyspark present in metadata, but the import succeeds (dbcImportErr == ""). + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0", pyspark: "4.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.True(t, res.OK) +} + func TestPipelineValidateRejectsUnparseableInstalledVersion(t *testing.T) { dir := writeProject(t) // sampleToml has databricks-connect~=17.2.0 as the pin; use an empty installed diff --git a/libs/localenv/pkgmanager.go b/libs/localenv/pkgmanager.go index 2dd407af37..91cc136b37 100644 --- a/libs/localenv/pkgmanager.go +++ b/libs/localenv/pkgmanager.go @@ -27,7 +27,31 @@ type PackageManager interface { // strips pip, so seeding must run after every sync. PostProvision(ctx context.Context, projectDir string) error - // Validate reads the Python minor version and databricks-connect version - // from the virtual environment inside projectDir. - Validate(ctx context.Context, projectDir string) (pythonVersion, dbconnectVersion string, err error) + // Validate inspects the provisioned virtual environment inside projectDir and + // returns what it observed (see VenvInfo). The caller decides which observations + // are acceptable. + Validate(ctx context.Context, projectDir string) (VenvInfo, error) +} + +// VenvInfo is what the validate phase observed in the provisioned virtual environment. +type VenvInfo struct { + // PythonMinor is the interpreter's "major.minor" (e.g. "3.12"). + PythonMinor string + // DBConnect is the installed databricks-connect distribution version, "" if absent. + DBConnect string + // Pyspark is the installed standalone pyspark distribution version, "" if absent. + // databricks-connect vendors the pyspark package tree without registering a pyspark + // distribution, so a non-empty value means a separate pyspark distribution is + // installed on top of it. + Pyspark string + // DBConnectImportErr is the type name of the exception raised by `import + // databricks.connect` in the venv, or "" when the import succeeds. It is non-empty + // whenever the import raises — including ModuleNotFoundError when databricks-connect + // is not installed at all — so a reader must gate on DBConnect != "" before treating + // it as a collision signal. A *live* standalone-pyspark collision surfaces here as an + // ImportError, because the two share the pyspark namespace and the losing package's + // files are overwritten. A stale, orphaned pyspark dist-info left behind by an install + // that databricks-connect's files won does NOT set this — the import still succeeds — + // which is how the caller tells a broken collision from a harmless leftover. + DBConnectImportErr string } diff --git a/libs/localenv/uv.go b/libs/localenv/uv.go index dc8c5bb35e..7e4ca82c74 100644 --- a/libs/localenv/uv.go +++ b/libs/localenv/uv.go @@ -134,22 +134,36 @@ func (m *uvManager) PostProvision(ctx context.Context, projectDir string) error return nil } -// Validate reads the Python minor version and databricks-connect package -// version from the project's virtual environment. When databricks-connect is not -// installed (constraints-only mode), the second line is empty rather than an -// error: PackageNotFoundError is caught so the probe never fails just because the -// package is absent. The caller decides whether an empty version is acceptable. -func (m *uvManager) Validate(ctx context.Context, projectDir string) (string, string, error) { - // Each value is printed with a unique prefix so parsing greps for the prefix - // rather than relying on line position: any stray line uv or the interpreter - // writes to stdout (e.g. a warning) would otherwise shift a positional parse. - // A missing databricks-connect prints an empty DBC: value, not an error. - pyCode := `import sys, importlib.metadata +// Validate inspects the project's virtual environment: the Python minor version, the +// databricks-connect and standalone-pyspark distribution versions, and whether +// databricks-connect actually imports. A missing databricks-connect or pyspark yields +// an empty version rather than an error (PackageNotFoundError is caught), so the probe +// never fails just because a package is absent; the caller decides what is acceptable. +// +// The version probes read distribution metadata, not the importable module: +// databricks-connect vendors the pyspark package tree without registering a pyspark +// distribution, so a resolvable pyspark version means a standalone pyspark is installed +// on top of it. Metadata alone cannot tell a live collision from a stale dist-info that +// no longer describes the importable module, so the probe also attempts `import +// databricks.connect`: a live collision raises there, a harmless leftover does not. +func (m *uvManager) Validate(ctx context.Context, projectDir string) (VenvInfo, error) { + // Each value is printed with a unique prefix so parsing greps for the prefix rather + // than relying on line position: any stray line uv or the interpreter writes to + // stdout (e.g. a warning) would otherwise shift a positional parse. + pyCode := `import sys, importlib, importlib.metadata +def _ver(name): + try: + return importlib.metadata.version(name) + except importlib.metadata.PackageNotFoundError: + return "" print(f"` + validatePyPrefix + `{sys.version_info.major}.{sys.version_info.minor}") +print("` + validateDBCPrefix + `" + _ver("databricks-connect")) +print("` + validatePysparkPrefix + `" + _ver("pyspark")) try: - print("` + validateDBCPrefix + `" + importlib.metadata.version("databricks-connect")) -except importlib.metadata.PackageNotFoundError: - print("` + validateDBCPrefix + `")` + importlib.import_module("databricks.connect") + print("` + validateDBCImportPrefix + `") +except BaseException as e: + print("` + validateDBCImportPrefix + `" + type(e).__name__)` // Invoke the venv interpreter directly rather than `uv run`: `uv run` resolves // the interpreter from an active VIRTUAL_ENV / CONDA_PREFIX when one is set // (even with --no-project), which would validate whatever env the caller has @@ -161,22 +175,32 @@ except importlib.metadata.PackageNotFoundError: process.WithProcessGroup(), ) if err != nil { - return "", "", uvFailure(ErrValidate, err, "venv python validation") + return VenvInfo{}, uvFailure(ErrValidate, err, "venv python validation") } pyVer, ok := lineWithPrefix(out, validatePyPrefix) if !ok || pyVer == "" { - return "", "", NewError(ErrValidate, nil, "unexpected output from uv run: %q", out) + return VenvInfo{}, NewError(ErrValidate, nil, "unexpected output from uv run: %q", out) } - // The databricks-connect value is empty when the package is not installed. + // databricks-connect / pyspark versions are empty when the package is not installed + // as a distribution of its own; the import-error line is empty when the import worked. dbcVer, _ := lineWithPrefix(out, validateDBCPrefix) - return pyVer, dbcVer, nil + pysparkVer, _ := lineWithPrefix(out, validatePysparkPrefix) + dbcImportErr, _ := lineWithPrefix(out, validateDBCImportPrefix) + return VenvInfo{ + PythonMinor: pyVer, + DBConnect: dbcVer, + Pyspark: pysparkVer, + DBConnectImportErr: dbcImportErr, + }, nil } // Validation output prefixes: uv run's stdout is grepped for these rather than // parsed positionally, so extra lines from uv or the interpreter don't break it. const ( - validatePyPrefix = "PYVER:" - validateDBCPrefix = "DBCVER:" + validatePyPrefix = "PYVER:" + validateDBCPrefix = "DBCVER:" + validatePysparkPrefix = "PYSPARKVER:" + validateDBCImportPrefix = "DBCIMPORT:" ) // lineWithPrefix returns the trimmed remainder of the first line in out that