You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/12/08 22:00:39 UTC
[GitHub] [beam] emilymye opened a new pull request #13508: Better logging for Python Docker environment
emilymye opened a new pull request #13508:
URL: https://github.com/apache/beam/pull/13508
As we introduce custom containers (mostly for Python), there are a couple of misleading or unclear log messages.
* PortableRunner currently attempts to pull the container and ignores errors, defaulting to local images. Logging added to make this path clearer.
* DockerEnvironment.default_docker_image can be called even if we don't use the default image - currently this outputs a message saying `Using default image "..."` which is very confusing. I edited to make sure it's clearer when we are using provided container image vs default container image.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] 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/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam
.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
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)
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] emilymye commented on pull request #13508: Better logging for Python Docker environment
Posted by GitBox <gi...@apache.org>.
emilymye commented on pull request #13508:
URL: https://github.com/apache/beam/pull/13508#issuecomment-741241298
R: @ibzib
R: @tvalentyn
CC: @tysonjh
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #13508: Better logging for Python Docker environment
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #13508:
URL: https://github.com/apache/beam/pull/13508#discussion_r538907209
##########
File path: sdks/python/apache_beam/transforms/environments.py
##########
@@ -240,15 +240,16 @@ def __init__(
super(DockerEnvironment, self).__init__(capabilities, artifacts)
if container_image:
logging.info(
- 'Using provided Python SDK container image: %s' % (container_image))
+ 'Using provided Python SDK container image: %s' % (container_image))
self.container_image = container_image
else:
- logging.info('No image given - inferring default Python SDK container image')
+ logging.info(
+ 'No image given - inferring default Python SDK container image')
Review comment:
I prefer not to say `container_image` since it could be misconstrued as a flag. Best to leave it how you had it originally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a change in pull request #13508: Better logging for Python Docker environment
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on a change in pull request #13508:
URL: https://github.com/apache/beam/pull/13508#discussion_r539793778
##########
File path: sdks/python/apache_beam/transforms/environments.py
##########
@@ -312,9 +319,7 @@ def default_docker_image():
image = (
'apache/beam_python{version_suffix}_sdk:{tag}'.format(
version_suffix=version_suffix, tag=sdk_version))
- logging.info(
- 'Using Python SDK docker image: %s. If the image is not '
- 'available at local, we will try to pull from hub.docker.com' % (image))
+ logging.info('Default Python SDK image for environment is %s' % (image))
Review comment:
thanks for fixing this!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib merged pull request #13508: Better logging for Python Docker environment
Posted by GitBox <gi...@apache.org>.
ibzib merged pull request #13508:
URL: https://github.com/apache/beam/pull/13508
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] emilymye commented on a change in pull request #13508: Better logging for Python Docker environment
Posted by GitBox <gi...@apache.org>.
emilymye commented on a change in pull request #13508:
URL: https://github.com/apache/beam/pull/13508#discussion_r538865483
##########
File path: sdks/python/apache_beam/transforms/environments.py
##########
@@ -240,15 +240,16 @@ def __init__(
super(DockerEnvironment, self).__init__(capabilities, artifacts)
if container_image:
logging.info(
- 'Using provided Python SDK container image: %s' % (container_image))
+ 'Using provided Python SDK container image: %s' % (container_image))
self.container_image = container_image
else:
- logging.info('No image given - inferring default Python SDK container image')
+ logging.info(
+ 'No image given - inferring default Python SDK container image')
Review comment:
hmm I'm not sure it's a good idea to link the possible env/runner flags to the log message here, but at least changed the wording a bit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #13508: Better logging for Python Docker environment
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #13508:
URL: https://github.com/apache/beam/pull/13508#discussion_r538847386
##########
File path: sdks/python/apache_beam/transforms/environments.py
##########
@@ -240,15 +240,16 @@ def __init__(
super(DockerEnvironment, self).__init__(capabilities, artifacts)
if container_image:
logging.info(
- 'Using provided Python SDK container image: %s' % (container_image))
+ 'Using provided Python SDK container image: %s' % (container_image))
self.container_image = container_image
else:
- logging.info('No image given - inferring default Python SDK container image')
+ logging.info(
+ 'No image given - inferring default Python SDK container image')
Review comment:
This would be a good place to tell the user how to provide an image if they want to.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org