You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "AnandInguva (via GitHub)" <gi...@apache.org> on 2023/07/10 19:36:40 UTC
[GitHub] [beam] AnandInguva opened a new pull request, #27436: Don't stage beam SDK in Stager
AnandInguva opened a new pull request, #27436:
URL: https://github.com/apache/beam/pull/27436
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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
[GitHub] [beam] tvalentyn commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1650078270
Run Python Dataflow ValidatesContainer
--
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
[GitHub] [beam] tvalentyn commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1662694253
hmm.. that job has errors like:
```
DEBUG 2023-08-02T07:02:33.534971077Z Executing Python (worker sdk-0-0): python -m apache_beam.runners.worker.sdk_worker_main
INFO 2023-08-02T07:02:33.596045792Z /opt/apache/beam-venv/beam-venv-worker-sdk-0-0/bin/python: Error while finding module specification for 'apache_beam.runners.worker.sdk_worker_main' (ModuleNotFoundError: No module named 'apache_beam')
INFO 2023-08-02T07:02:33.596088772Z /opt/apache/beam-venv/beam-venv-worker-sdk-0-0/bin/python: Error while finding module specification for 'apache_beam.runners.worker.sdk_worker_main' (ModuleNotFoundError: No module named 'apache_beam')
```
--
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
[GitHub] [beam] AnandInguva commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1277037475
##########
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:
here, the Run method will return an exec.ExitError value so yes
--
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
[GitHub] [beam] riteshghorse commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "riteshghorse (via GitHub)" <gi...@apache.org>.
riteshghorse commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1261311334
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -296,30 +290,24 @@ 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
+ if Stager._is_remote_path(sdk_location):
+ try:
+ resources.extend(
+ Stager._create_beam_sdk(
+ sdk_remote_location=setup_options.sdk_location,
+ temp_dir=temp_dir,
+ ))
+ except:
+ raise RuntimeError(
+ 'The --sdk_location option was used with an unsupported '
+ 'type of location: %s' % sdk_location)
+
+ elif sdk_location == 'default' or sdk_location == 'container':
Review Comment:
can we still keep the comment here
```
# Use the SDK that's built into the container, rather than re-staging it.
```
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -296,30 +290,24 @@ 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
+ if Stager._is_remote_path(sdk_location):
+ try:
+ resources.extend(
+ Stager._create_beam_sdk(
+ sdk_remote_location=setup_options.sdk_location,
+ temp_dir=temp_dir,
+ ))
+ except:
+ raise RuntimeError(
+ 'The --sdk_location option was used with an unsupported '
+ 'type of location: %s' % sdk_location)
+
+ elif sdk_location == 'default' or sdk_location == 'container':
pass
+
Review Comment:
rm spare line
--
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
[GitHub] [beam] github-actions[bot] commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1632909114
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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
[GitHub] [beam] tvalentyn commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1275186021
##########
sdks/python/apache_beam/options/pipeline_options.py:
##########
@@ -1284,11 +1284,11 @@ def _add_argparse_args(cls, parser):
'--sdk_location',
default='default',
help=(
- 'Override the default location from where the Beam SDK is '
- 'downloaded. It can be a URL, a GCS path, or a local path to an '
+ 'Override the default location of the beam SDK. '
Review Comment:
```suggestion
'Path to a custom Beam SDK package to install and use on the runner. '
```
##########
CHANGES.md:
##########
@@ -72,6 +72,7 @@
## Breaking Changes
* Legacy runner support removed from Dataflow, all pipelines must use runner v2.
+* [Python]Beam SDK will not be staged from PyPI in the --staging_location for DataflowRunner. If the custom container doesn't include Apache Beam, it will no longer be installed using the staged Beam SDK.([#26996](https://github.com/apache/beam/issues/26996))
Review Comment:
```suggestion
* [Python] Dataflow Runner will no longer stage Beam SDK from PyPI in the `--staging_location` at pipeline submission. Custom container images that are not based on Beam's default image must include Apache Beam installation.([#26996](https://github.com/apache/beam/issues/26996))
```
##########
sdks/python/apache_beam/options/pipeline_options.py:
##########
@@ -1284,11 +1284,11 @@ def _add_argparse_args(cls, parser):
'--sdk_location',
default='default',
help=(
- 'Override the default location from where the Beam SDK is '
- 'downloaded. It can be a URL, a GCS path, or a local path to an '
+ 'Override the default location of the beam SDK. '
+ 'It can be a URL, a GCS path, or a local path to an '
'SDK tarball. Workflow submissions will download or copy an SDK '
Review Comment:
Use this flag when running pipelines with an unreleased or manually patched version of Beam SDK.
##########
sdks/python/apache_beam/options/pipeline_options.py:
##########
@@ -1284,11 +1284,11 @@ def _add_argparse_args(cls, parser):
'--sdk_location',
default='default',
help=(
- 'Override the default location from where the Beam SDK is '
- 'downloaded. It can be a URL, a GCS path, or a local path to an '
+ 'Override the default location of the beam SDK. '
+ 'It can be a URL, a GCS path, or a local path to an '
'SDK tarball. Workflow submissions will download or copy an SDK '
- 'tarball from here. If set to the string "default", a standard '
- 'SDK location is used. If empty, no SDK is copied.'))
+ 'tarball from here. If set to the string "default", '
+ 'the beam SDK in the default container will be used.'))
Review Comment:
If set to the string "default", runners will use the SDK provided in the default environment.
##########
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:
this should actually be an error.
##########
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 {
+ log.Printf("Apache Beam is not installed on the custom container. Please make sure Apache Beam is installed in the custom container. Look at the docummentation https://beam.apache.org/documentation/runtime/environments/ on custom containers for more details.")
Review Comment:
wording suggestion: "Apache Beam is not installed in the runtime environment. If you use a custom container image, you must install apache-beam package in the custom image using same version of Beam as in the pipeline submission environment. For more information, see: the https://beam.apache.org/documentation/runtime/environments/ page."
##########
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)
Review Comment:
this shouldn't be a hard error, we can attempt to continue.
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -296,30 +290,24 @@ 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
+ if Stager._is_remote_path(sdk_location):
+ try:
+ resources.extend(
+ Stager._create_beam_sdk(
+ sdk_remote_location=setup_options.sdk_location,
+ temp_dir=temp_dir,
+ ))
+ except:
+ raise RuntimeError(
+ 'The --sdk_location option was used with an unsupported '
+ 'type of location: %s' % sdk_location)
+
+ elif sdk_location == 'default' or sdk_location == 'container':
Review Comment:
```
elif sdk_location == 'default':
# Use default location for a runner.
pass
elif sdk_location == 'container':
# Used in the past to indicate that SDK should be used from container image instead of being staged.
# Equivalent to 'default' now, leaving for backwards compatibility reasons.
pass
```
##########
sdks/python/container/piputil.go:
##########
@@ -52,6 +53,23 @@ func pipInstallRequirements(files []string, dir, name string) error {
return nil
}
+
+func isPackageInstalled(pkgName string) (bool, error) {
Review Comment:
`pip show` would be a better way.
substring searches would confuse `tensorflow` for `tensorflow-metadata` for example.
--
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
[GitHub] [beam] tvalentyn commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1656469918
Run Python_Runners PreCommit
--
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
[GitHub] [beam] AnandInguva commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1650032062
Run Python Dataflow ValidatesContainer
--
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
[GitHub] [beam] AnandInguva commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1629730141
Run Python 3.10 PostCommit
--
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
[GitHub] [beam] AnandInguva commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1284056421
##########
CHANGES.md:
##########
@@ -76,7 +76,8 @@
## Breaking Changes
-* Python SDK: Legacy runner support removed from Dataflow, all pipelines must use runner v2.
+* Legacy runner support removed from Dataflow, all pipelines must use runner v2.
Review Comment:
```suggestion
* Python SDK: Legacy runner support removed from Dataflow, all pipelines must use runner v2.
```
--
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
[GitHub] [beam] AnandInguva commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1275672851
##########
sdks/python/apache_beam/options/pipeline_options.py:
##########
@@ -1284,11 +1284,11 @@ def _add_argparse_args(cls, parser):
'--sdk_location',
default='default',
help=(
- 'Override the default location from where the Beam SDK is '
- 'downloaded. It can be a URL, a GCS path, or a local path to an '
+ 'Override the default location of the beam SDK. '
+ 'It can be a URL, a GCS path, or a local path to an '
'SDK tarball. Workflow submissions will download or copy an SDK '
Review Comment:
reworded
--
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
[GitHub] [beam] github-actions[bot] commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1629762219
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @riteshghorse for label python.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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
[GitHub] [beam] AnandInguva commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1275667763
##########
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)
Review Comment:
I removed this entirely.
##########
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 {
+ log.Printf("Apache Beam is not installed on the custom container. Please make sure Apache Beam is installed in the custom container. Look at the docummentation https://beam.apache.org/documentation/runtime/environments/ on custom containers for more details.")
Review Comment:
Changed wording
--
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
[GitHub] [beam] AnandInguva commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1261480570
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -296,30 +290,24 @@ 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
+ if Stager._is_remote_path(sdk_location):
+ try:
+ resources.extend(
+ Stager._create_beam_sdk(
+ sdk_remote_location=setup_options.sdk_location,
+ temp_dir=temp_dir,
+ ))
+ except:
+ raise RuntimeError(
+ 'The --sdk_location option was used with an unsupported '
+ 'type of location: %s' % sdk_location)
+
+ elif sdk_location == 'default' or sdk_location == 'container':
Review Comment:
Sg. I will re-add the comment along with some more context
--
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
[GitHub] [beam] tvalentyn merged pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn merged PR #27436:
URL: https://github.com/apache/beam/pull/27436
--
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
[GitHub] [beam] tvalentyn commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1656470773
Thanks, changes look good to me. have you tested them in positive and negative case? Negative case requries building a custom docker image similar to https://cloud.google.com/dataflow/docs/guides/using-custom-containers#use_a_custom_base_image_or_multi-stage_builds but w/o Beam.
If you are already OOO i can TAL next week, np.
--
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
[GitHub] [beam] AnandInguva commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1661563909
> Thanks, changes look good to me. have you tested them in positive and negative case? Negative case requries building a custom docker image similar to https://cloud.google.com/dataflow/docs/guides/using-custom-containers#use_a_custom_base_image_or_multi-stage_builds but w/o Beam.
>
> If you are already OOO i can TAL next week, np.
This job uses the container gcr.io/google.com/clouddfe/anandinguva:2.50.0.dev with beam installed in it. I can the see the message in the logs but at `INFO` level.
https://pantheon.corp.google.com/dataflow/jobs/us-central1/2023-08-01_23_06_17-11941549679963536368;mainTab=JOB_GRAPH;bottomTab=WORKER_LOGS;logsSeverity=INFO;graphView=0?project=google.com:clouddfe&pageState=(%22dfTime%22:(%22l%22:%22dfJobMaxTime%22))
--
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
[GitHub] [beam] AnandInguva commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1632907504
R: @tvalentyn
--
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
[GitHub] [beam] tvalentyn commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1633240222
- let's announce a breaking change in changes.md
- let's add a check in boot.go that would fail with a meaningful error if a (custom) container doesn't include beam.
--
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
[GitHub] [beam] codecov[bot] commented on pull request #27436: Don't stage beam SDK in Stager
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1629685680
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/27436?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#27436](https://app.codecov.io/gh/apache/beam/pull/27436?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (44b3984) into [master](https://app.codecov.io/gh/apache/beam/commit/c06247bf79ec3fd02810c26b7f0da58f6a790892?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c06247b) will **decrease** coverage by `0.03%`.
> The diff coverage is `85.71%`.
```diff
@@ Coverage Diff @@
## master #27436 +/- ##
==========================================
- Coverage 71.33% 71.31% -0.03%
==========================================
Files 862 862
Lines 105206 105162 -44
==========================================
- Hits 75050 74996 -54
- Misses 28608 28618 +10
Partials 1548 1548
```
| Flag | Coverage Δ | |
|---|---|---|
| python | `80.51% <85.71%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/beam/pull/27436?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...s/python/apache\_beam/runners/portability/stager.py](https://app.codecov.io/gh/apache/beam/pull/27436?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zdGFnZXIucHk=) | `84.29% <85.71%> (-2.75%)` | :arrow_down: |
... and [9 files with indirect coverage changes](https://app.codecov.io/gh/apache/beam/pull/27436/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [beam] AnandInguva commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1665086522
https://pantheon.corp.google.com/dataflow/jobs/us-central1/2023-08-03_23_16_40-2480228236867561239;bottomTab=WORKER_LOGS;logsSeverity=INFO;graphView=0?project=google.com:clouddfe&pageState=(%22dfTime%22:(%22l%22:%22dfJobMaxTime%22))
This is the latest job with the updated boot.go. This is working as expected as we can see the expected errors in the logs.
--
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
[GitHub] [beam] AnandInguva commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1273745486
##########
sdks/python/container/piputil.go:
##########
@@ -52,6 +53,26 @@ func pipInstallRequirements(files []string, dir, name string) error {
return nil
}
+
+func isPackageInstalled(pkgName string) (bool, error) {
+ // run pip list and capture the output
Review Comment:
```suggestion
```
##########
sdks/python/container/piputil.go:
##########
@@ -52,6 +53,26 @@ func pipInstallRequirements(files []string, dir, name string) error {
return nil
}
+
+func isPackageInstalled(pkgName string) (bool, error) {
+ // run pip list and capture the output
+ out, err := exec.Command("python", "-m", "pip", "list").Output()
+ if err != nil {
+ return false, fmt.Errorf("failed to fetch pip list: %v", err)
+ }
+
+ // Convert byte array to string
Review Comment:
```suggestion
```
##########
sdks/python/container/piputil.go:
##########
@@ -52,6 +53,26 @@ func pipInstallRequirements(files []string, dir, name string) error {
return nil
}
+
+func isPackageInstalled(pkgName string) (bool, error) {
+ // run pip list and capture the output
+ out, err := exec.Command("python", "-m", "pip", "list").Output()
+ if err != nil {
+ return false, fmt.Errorf("failed to fetch pip list: %v", err)
+ }
+
+ // Convert byte array to string
+ output := string(out[:])
+
+ // Check if package name is in the output
Review Comment:
```suggestion
```
--
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
[GitHub] [beam] AnandInguva commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1651192103
Run Python RC Dataflow ValidatesContainer
--
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
[GitHub] [beam] riteshghorse commented on a diff in pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "riteshghorse (via GitHub)" <gi...@apache.org>.
riteshghorse commented on code in PR #27436:
URL: https://github.com/apache/beam/pull/27436#discussion_r1273594122
##########
sdks/python/container/boot.go:
##########
@@ -371,6 +371,15 @@ func installSetupPackages(files []string, workDir string, requirementsFiles []st
log.Printf("Failed to setup acceptable wheel specs, leave it as empty: %v", err)
}
+ isSdkInstalled, err := isPackageInstalled(pkgName)
Review Comment:
`pkgName` is not defined here
--
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
[GitHub] [beam] AnandInguva commented on pull request #27436: [Python]Don't stage beam SDK in Stager
Posted by "AnandInguva (via GitHub)" <gi...@apache.org>.
AnandInguva commented on PR #27436:
URL: https://github.com/apache/beam/pull/27436#issuecomment-1629730319
Run Python 3.11 PostCommit
--
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