tvalentyn commented on code in PR #28046:
URL: https://github.com/apache/beam/pull/28046#discussion_r1300506160
##########
sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download.go:
##########
@@ -382,7 +382,14 @@ func jarExists(jarPath string) bool {
return err == nil
}
-func getPythonVersion() (string, error) {
+// GetPythonVersion returns the Python version to use. It checks for
+// env variable PYTHON_PATH and returns that it if set.
+// If no PYTHON_PATH is defined then it checks for python or python3
Review Comment:
Let's modify the error message on line 399 to say sth like:
no python installation found. If you use a custom container image, please
verify that python or python3 command launches the interpreter or specify the
full path to the interpreter file in PYTHON_PATH environment variable.
##########
sdks/python/container/boot.go:
##########
@@ -91,7 +92,12 @@ func main() {
"--container_executable=/opt/apache/beam/boot",
}
log.Printf("Starting worker pool %v: python %v", workerPoolId,
strings.Join(args, " "))
- if err := execx.Execute("python", args...); err != nil {
+ pythonVersion, err := expansionx.GetPythonVersion()
+ if err != nil {
+ log.Print("Python SDK worker pool exited.")
Review Comment:
Can we buble up the "no python installation found" error instead?
Should we use `log.Fatalf` instead?
##########
sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download_test.go:
##########
@@ -217,3 +217,31 @@ func TestGetJar_dev(t *testing.T) {
t.Errorf("error message does not contain gradle command %v for
user, got message: %v", gradleTarget, err)
}
}
+
+func TestGetPythonVersion(t *testing.T) {
+ tests := []struct {
+ name string
+ PYTHON_PATH string
+ }{
+ {
+ name: "PYTHON_PATH set",
+ PYTHON_PATH: "/bin/python",
+ },
+ {
+ name: "PYTHON_PATH not set",
+ PYTHON_PATH: "",
+ },
+ }
+
+ for _, test := range tests {
+ os.Setenv("PYTHON_PATH", test.PYTHON_PATH)
Review Comment:
Consider setting only if not empty.
##########
sdks/python/container/boot.go:
##########
@@ -336,7 +342,11 @@ func setupVenv(ctx context.Context, logger *tools.Logger,
baseDir, workerId stri
if err := os.MkdirAll(dir, 0750); err != nil {
return "", fmt.Errorf("failed to create Python venv directory:
%s", err)
}
- if err := execx.Execute("python", "-m", "venv",
"--system-site-packages", dir); err != nil {
+ pythonVersion, err := expansionx.GetPythonVersion()
+ if err != nil {
+ return "", fmt.Errorf("python installation not found: %v", err)
Review Comment:
should we return `err` without modifications here this message is already
included?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
