ryanthompson591 commented on a change in pull request #16658:
URL: https://github.com/apache/beam/pull/16658#discussion_r800771715



##########
File path: sdks/python/container/boot.go
##########
@@ -145,7 +145,21 @@ func main() {
        // Guard from concurrent artifact retrieval and installation,
        // when called by child processes in a worker pool.
 
+       workerPoolId := os.Getenv(workerPoolIdEnv)
+       var venvDir string
+       if workerPoolId != "" {
+               venvDir = filepath.Join(*semiPersistDir, "beam-venv", 
"beam-pool-" + workerPoolId)
+       } else {
+               venvDir = filepath.Join(*semiPersistDir, "beam-venv", 
"beam-worker-" + *id)

Review comment:
       Yeah, I guess I'm more making a suggestion to add a comment for less 
informed people like myself in the code here.  If you think someone reading 
this code would understand it without a comment then feel free to ignore my 
suggestion (I'm just saying I didn't understand).

##########
File path: sdks/python/container/boot.go
##########
@@ -210,15 +224,29 @@ func main() {
        wg.Add(len(workerIds))
        for _, workerId := range workerIds {
                go func(workerId string) {
+                       defer wg.Done()
                        log.Printf("Executing: python %v", strings.Join(args, " 
"))
-                       log.Fatalf("Python exited: %v", 
execx.ExecuteEnv(map[string]string{"WORKER_ID": workerId}, "python", args...))
-                       wg.Done()
+                       log.Printf("Python exited: %v", 
execx.ExecuteEnv(map[string]string{"WORKER_ID": workerId}, "python", args...))
                }(workerId)
        }
        wg.Wait()
 }
 
-// setup wheel specs according to installed python version
+// setupVenv initialize a local Python venv and set the corresponding env 
variables
+func setupVenv(dir string) error {
+       log.Printf("Initializing temporary Python venv ...")
+       if _, err := os.Stat(filepath.Join(dir, "pyenv.cfg")); 
os.IsNotExist(err) {

Review comment:
       So the way I read this code is,
   if os.Stat returns an error then only take any action on that error if 
os.IsNotExist, other errors will silently continue.
   
   If that's what we want then this is probably fine.




-- 
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]


Reply via email to