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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
