From c7a1e9130310cb25ee60cc478c133303682e83b2 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Fri, 16 Aug 2024 14:33:38 +0200 Subject: [PATCH 1/5] Make venv optional in PyDABs --- bundle/artifacts/whl/infer.go | 2 + .../config/mutator/python/python_mutator.go | 34 +++++++------- .../mutator/python/python_mutator_test.go | 21 +++++++-- libs/python/detect.go | 40 ++++++++++++++++ libs/python/detect_test.go | 46 +++++++++++++++++++ 5 files changed, 121 insertions(+), 22 deletions(-) create mode 100644 libs/python/detect_test.go diff --git a/bundle/artifacts/whl/infer.go b/bundle/artifacts/whl/infer.go index dd4ad2956d..cb727de0ee 100644 --- a/bundle/artifacts/whl/infer.go +++ b/bundle/artifacts/whl/infer.go @@ -15,6 +15,8 @@ type infer struct { func (m *infer) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { artifact := b.Config.Artifacts[m.name] + + // TODO use python.DetectVEnvExecutable once bundle has a way to specify venv path py, err := python.DetectExecutable(ctx) if err != nil { return diag.FromErr(err) diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index f9febe5b5d..8414a0c4fa 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -7,8 +7,8 @@ import ( "fmt" "os" "path/filepath" - "runtime" + "github.com/databricks/cli/libs/python" "github.com/databricks/databricks-sdk-go/logger" "github.com/databricks/cli/bundle/env" @@ -86,23 +86,15 @@ func (m *pythonMutator) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagno return nil } - if experimental.PyDABs.VEnvPath == "" { - return diag.Errorf("\"experimental.pydabs.enabled\" can only be used when \"experimental.pydabs.venv_path\" is set") - } - // mutateDiags is used because Mutate returns 'error' instead of 'diag.Diagnostics' var mutateDiags diag.Diagnostics var mutateDiagsHasError = errors.New("unexpected error") err := b.Config.Mutate(func(leftRoot dyn.Value) (dyn.Value, error) { - pythonPath := interpreterPath(experimental.PyDABs.VEnvPath) + pythonPath, err := detectExecutable(ctx, experimental.PyDABs.VEnvPath) - if _, err := os.Stat(pythonPath); err != nil { - if os.IsNotExist(err) { - return dyn.InvalidValue, fmt.Errorf("can't find %q, check if venv is created", pythonPath) - } else { - return dyn.InvalidValue, fmt.Errorf("can't find %q: %w", pythonPath, err) - } + if err != nil { + return dyn.InvalidValue, fmt.Errorf("failed to get Python interpreter path: %w", err) } cacheDir, err := createCacheDir(ctx) @@ -423,11 +415,17 @@ func isOmitemptyDelete(left dyn.Value) bool { } } -// interpreterPath returns platform-specific path to Python interpreter in the virtual environment. -func interpreterPath(venvPath string) string { - if runtime.GOOS == "windows" { - return filepath.Join(venvPath, "Scripts", "python3.exe") - } else { - return filepath.Join(venvPath, "bin", "python3") +// detectExecutable lookups Python interpreter in virtual environment, or if not set, in PATH. +func detectExecutable(ctx context.Context, venvPath string) (string, error) { + if venvPath == "" { + interpreter, err := python.DetectExecutable(ctx) + + if err != nil { + return "", err + } + + return interpreter, nil } + + return python.DetectVEnvExecutable(venvPath) } diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index fbe835f928..ea02d1cede 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -282,7 +282,7 @@ func TestPythonMutator_venvRequired(t *testing.T) { } func TestPythonMutator_venvNotFound(t *testing.T) { - expectedError := fmt.Sprintf("can't find %q, check if venv is created", interpreterPath("bad_path")) + expectedError := fmt.Sprintf("failed to get Python interpreter path: can't find %q, check if virtualenv is created", interpreterPath("bad_path")) b := loadYaml("databricks.yml", ` experimental: @@ -596,9 +596,7 @@ func loadYaml(name string, content string) *bundle.Bundle { } } -func withFakeVEnv(t *testing.T, path string) { - interpreterPath := interpreterPath(path) - +func withFakeVEnv(t *testing.T, venvPath string) { cwd, err := os.Getwd() if err != nil { panic(err) @@ -608,6 +606,8 @@ func withFakeVEnv(t *testing.T, path string) { panic(err) } + interpreterPath := interpreterPath(venvPath) + err = os.MkdirAll(filepath.Dir(interpreterPath), 0755) if err != nil { panic(err) @@ -618,9 +618,22 @@ func withFakeVEnv(t *testing.T, path string) { panic(err) } + err = os.WriteFile(filepath.Join(venvPath, "pyvenv.cfg"), []byte(""), 0755) + if err != nil { + panic(err) + } + t.Cleanup(func() { if err := os.Chdir(cwd); err != nil { panic(err) } }) } + +func interpreterPath(venvPath string) string { + if runtime.GOOS == "windows" { + return filepath.Join(venvPath, "Scripts", "python3.exe") + } else { + return filepath.Join(venvPath, "bin", "python3") + } +} diff --git a/libs/python/detect.go b/libs/python/detect.go index b0c1475c0e..f183242c7a 100644 --- a/libs/python/detect.go +++ b/libs/python/detect.go @@ -3,9 +3,23 @@ package python import ( "context" "errors" + "fmt" + "io/fs" + "os" "os/exec" + "path/filepath" + "runtime" ) +// DetectExecutable looks up the path to the python3 executable from the PATH +// environment variable. +// +// If virtualenv is activated, executable from the virtualenv is returned, +// because activating virtualenv adds python3 executable on a PATH. +// +// If python3 executable is not found on the PATH, the interpreter with the +// least version that satisfies minimal 3.8 version is returned, e.g. +// python3.10. func DetectExecutable(ctx context.Context) (string, error) { // TODO: add a shortcut if .python-version file is detected somewhere in // the parent directory tree. @@ -32,3 +46,29 @@ func DetectExecutable(ctx context.Context) (string, error) { } return interpreter.Path, nil } + +// DetectVEnvExecutable returns the path to the python3 executable inside venvPath, +// that is not necessarily activated. +// +// If virtualenv is not created, or executable doesn't exist, the error is returned. +func DetectVEnvExecutable(venvPath string) (string, error) { + interpreterPath := filepath.Join(venvPath, "bin", "python3") + if runtime.GOOS == "windows" { + interpreterPath = filepath.Join(venvPath, "Scripts", "python3.exe") + } + + if _, err := os.Stat(interpreterPath); err != nil { + if os.IsNotExist(err) { + return "", fmt.Errorf("can't find %q, check if virtualenv is created", interpreterPath) + } else { + return "", fmt.Errorf("can't find %q: %w", interpreterPath, err) + } + } + + _, err := os.Stat(filepath.Join(venvPath, "pyvenv.cfg")) + if errors.Is(err, fs.ErrNotExist) { + return "", fmt.Errorf("expected %q to be virtualenv, but pyenv.cfg is missing", venvPath) + } + + return interpreterPath, nil +} diff --git a/libs/python/detect_test.go b/libs/python/detect_test.go new file mode 100644 index 0000000000..78c7067f7e --- /dev/null +++ b/libs/python/detect_test.go @@ -0,0 +1,46 @@ +package python + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDetectVEnvExecutable(t *testing.T) { + dir := t.TempDir() + interpreterPath := interpreterPath(dir) + + err := os.Mkdir(filepath.Dir(interpreterPath), 0755) + require.NoError(t, err) + + err = os.WriteFile(interpreterPath, []byte(""), 0755) + require.NoError(t, err) + + err = os.WriteFile(filepath.Join(dir, "pyvenv.cfg"), []byte(""), 0755) + require.NoError(t, err) + + executable, err := DetectVEnvExecutable(dir) + + assert.NoError(t, err) + assert.Equal(t, interpreterPath, executable) +} + +func TestDetectVEnvExecutable_badLayout(t *testing.T) { + dir := t.TempDir() + + _, err := DetectVEnvExecutable(dir) + + assert.Errorf(t, err, "can't find %q, check if virtualenv is created", interpreterPath(dir)) +} + +func interpreterPath(venvPath string) string { + if runtime.GOOS == "windows" { + return filepath.Join(venvPath, "Scripts", "python3.exe") + } else { + return filepath.Join(venvPath, "bin", "python3") + } +} From 66e331b0d368441f35babb59fc9538b02c68a2ff Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Fri, 16 Aug 2024 14:48:39 +0200 Subject: [PATCH 2/5] Update comment --- bundle/config/experimental.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/experimental.go b/bundle/config/experimental.go index 12048a3227..ed8667a75a 100644 --- a/bundle/config/experimental.go +++ b/bundle/config/experimental.go @@ -36,8 +36,8 @@ type PyDABs struct { // VEnvPath is path to the virtual environment. // - // Required if PyDABs is enabled. PyDABs will load the code in the specified - // environment. + // If enabled, PyDABs will execute code within this environment. If disabled, + // it defaults to using the Python interpreter available in the current shell. VEnvPath string `json:"venv_path,omitempty"` } From d38912a3d580d86fb7d47effcc5470c50200767d Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Mon, 19 Aug 2024 16:41:36 +0200 Subject: [PATCH 3/5] Update comment --- bundle/config/mutator/python/python_mutator.go | 1 - libs/python/detect.go | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index 8414a0c4fa..4f44df0a99 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -419,7 +419,6 @@ func isOmitemptyDelete(left dyn.Value) bool { func detectExecutable(ctx context.Context, venvPath string) (string, error) { if venvPath == "" { interpreter, err := python.DetectExecutable(ctx) - if err != nil { return "", err } diff --git a/libs/python/detect.go b/libs/python/detect.go index f183242c7a..5aff2ff4cf 100644 --- a/libs/python/detect.go +++ b/libs/python/detect.go @@ -65,6 +65,8 @@ func DetectVEnvExecutable(venvPath string) (string, error) { } } + // pyvenv.cfg must be always present in correctly configured virtualenv, + // read more in https://snarky.ca/how-virtual-environments-work/ _, err := os.Stat(filepath.Join(venvPath, "pyvenv.cfg")) if errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("expected %q to be virtualenv, but pyenv.cfg is missing", venvPath) From e76cc7a95f93c7146e59346d6f3f3bf40cbdf948 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Mon, 19 Aug 2024 19:45:54 +0200 Subject: [PATCH 4/5] Address CR feedback --- libs/python/detect.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/libs/python/detect.go b/libs/python/detect.go index 5aff2ff4cf..43699b7782 100644 --- a/libs/python/detect.go +++ b/libs/python/detect.go @@ -67,9 +67,13 @@ func DetectVEnvExecutable(venvPath string) (string, error) { // pyvenv.cfg must be always present in correctly configured virtualenv, // read more in https://snarky.ca/how-virtual-environments-work/ - _, err := os.Stat(filepath.Join(venvPath, "pyvenv.cfg")) - if errors.Is(err, fs.ErrNotExist) { - return "", fmt.Errorf("expected %q to be virtualenv, but pyenv.cfg is missing", venvPath) + pyvenvPath := filepath.Join(venvPath, "pyvenv.cfg") + if _, err := os.Stat(pyvenvPath); err != nil { + if errors.Is(err, fs.ErrNotExist) { + return "", fmt.Errorf("expected %q to be virtualenv, but pyvenv.cfg is missing", venvPath) + } else { + return "", fmt.Errorf("can't find %q: %w", pyvenvPath, err) + } } return interpreterPath, nil From 713f260a8b1fcfeb35e091f30bd05bd0f0556118 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 20 Aug 2024 15:20:27 +0200 Subject: [PATCH 5/5] Update libs/python/detect.go Co-authored-by: Pieter Noordhuis --- libs/python/detect.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/python/detect.go b/libs/python/detect.go index 43699b7782..8fcc7cd9ca 100644 --- a/libs/python/detect.go +++ b/libs/python/detect.go @@ -58,7 +58,7 @@ func DetectVEnvExecutable(venvPath string) (string, error) { } if _, err := os.Stat(interpreterPath); err != nil { - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("can't find %q, check if virtualenv is created", interpreterPath) } else { return "", fmt.Errorf("can't find %q: %w", interpreterPath, err)