[GitHub] [beam] tvalentyn commented on a diff in pull request #28317: Add buffered logger to the Python bootloader

["tvalentyn (via GitHub)" <gi...@apache.org>]

tvalentyn commented on code in PR #28317:
URL: https://github.com/apache/beam/pull/28317#discussion_r1317575362


##########
sdks/go/pkg/beam/util/execx/exec.go:
##########
@@ -43,3 +44,20 @@ func ExecuteEnv(env map[string]string, prog string, args ...string) error {
 
 	return cmd.Run()
 }
+
+// ExecuteEnvWithIO runs the program with the given arguments with additional environment
+// variables. It attaches custom IO to the child process.
+func ExecuteEnvWithIO(env map[string]string, stdin io.Reader, stdout, stderr io.Writer, prog string, args ...string) error {

Review Comment:
   nit: ExecuteEnv can call ExecuteEnvWithIO  with default params set. 
   



##########
sdks/python/container/piputil.go:
##########
@@ -97,19 +107,34 @@ func pipInstallPackage(files []string, dir, name string, force, optional bool, e
 				// installed version will match the package specified, the package itself
 				// will not be reinstalled, but its dependencies will now be resolved and
 				// installed if necessary.  This achieves our goal outlined above.
-				args := []string{"-m", "pip", "install", "-q", "--no-cache-dir", "--disable-pip-version-check", "--upgrade", "--force-reinstall", "--no-deps",
+				args := []string{"-m", "pip", "install", "--no-cache-dir", "--disable-pip-version-check", "--upgrade", "--force-reinstall", "--no-deps",
 					filepath.Join(dir, packageSpec)}
-				err := execx.Execute(pythonVersion, args...)
+				err := execx.ExecuteEnvWithIO(nil, os.Stdin, bufLogger, bufLogger, pythonVersion, args...)

Review Comment:
   have you considered passing buffered logger to the `ExexcuteEnvWith...` function, and flush at appropriate level inside the helper? 



##########
sdks/go/pkg/beam/util/execx/exec.go:
##########
@@ -43,3 +44,20 @@ func ExecuteEnv(env map[string]string, prog string, args ...string) error {
 
 	return cmd.Run()
 }
+
+// ExecuteEnvWithIO runs the program with the given arguments with additional environment
+// variables. It attaches custom IO to the child process.
+func ExecuteEnvWithIO(env map[string]string, stdin io.Reader, stdout, stderr io.Writer, prog string, args ...string) error {
+	cmd := exec.Command(prog, args...)
+	cmd.Stdin = stdin

Review Comment:
   does it make sense to default to os.Stdin if stdin is nil, etc?



##########
sdks/python/container/boot.go:
##########
@@ -450,7 +450,7 @@ func processArtifactsInSetupOnlyMode() {
 		}
 		files[i] = filePayload.GetPath()
 	}
-	if setupErr := installSetupPackages(files, workDir, []string{requirementsFile}); setupErr != nil {
+	if setupErr := installSetupPackages(context.Background(), nil, files, workDir, []string{requirementsFile}); setupErr != nil {

Review Comment:
   how is 'nil' logger value handled? have you tested the container prebuilding workflow with this change?
   
   Note: we should also run python validatescontainer test suite that exercises it.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org