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


##########
sdks/python/container/boot.go:
##########
@@ -371,6 +371,16 @@ func installSetupPackages(files []string, workDir string, 
requirementsFiles []st
                log.Printf("Failed to setup acceptable wheel specs, leave it as 
empty: %v", err)
        }
 
+       pkgName := "apache-beam"
+       isSdkInstalled, err := isPackageInstalled(pkgName)
+       if err != nil {
+               return fmt.Errorf("failed to check if Apache Beam %s is 
installed: %v", pkgName, err)
+       }
+
+       if !isSdkInstalled {

Review Comment:
   isPackageInstalled returns a tuple.



##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -296,30 +290,29 @@ def create_job_resources(options,  # type: PipelineOptions
                 setup_options.extra_packages, temp_dir=temp_dir))
 
       if hasattr(setup_options, 'sdk_location'):
-
-        if (setup_options.sdk_location == 'default') or Stager._is_remote_path(
-            setup_options.sdk_location):
-          # If --sdk_location is not specified then the appropriate package
-          # will be obtained from PyPI (https://pypi.python.org) based on the
-          # version of the currently running SDK. If the option is
-          # present then no version matching is made and the exact URL or path
-          # is expected.
-          #
-          # Unit tests running in the 'python setup.py test' context will
-          # not have the sdk_location attribute present and therefore we
-          # will not stage SDK.
-          sdk_remote_location = 'pypi' if (
-              setup_options.sdk_location == 'default'
-          ) else setup_options.sdk_location
-          resources.extend(
-              Stager._create_beam_sdk(sdk_remote_location, temp_dir))
-        elif setup_options.sdk_location == 'container':
-          # Use the SDK that's built into the container, rather than re-staging
-          # it.
+        sdk_location = setup_options.sdk_location
+        # check if it is remote location

Review Comment:
   this comment doesn't add value. it's self-evident from code.



##########
sdks/python/container/piputil.go:
##########
@@ -52,6 +53,18 @@ func pipInstallRequirements(files []string, dir, name 
string) error {
        return nil
 }
 
+// isPackageInstalled checks if the given package is installed in the
+// environment.
+func isPackageInstalled(pkgName string) (bool, error) {
+       cmd := exec.Command("python", "-m", "pip", "show", pkgName)
+       if err := cmd.Run(); err != nil {

Review Comment:
   does non-zero exit code have `err != nil` ?



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