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 2021/02/09 23:18:21 UTC
[GitHub] [beam] KevinGG opened a new pull request #13944: [BEAM-10708] Support SqlTransform in container
KevinGG opened a new pull request #13944:
URL: https://github.com/apache/beam/pull/13944
1. Changed how Linux system exposes worker handler endpoints to
sibling Java docker containers when executing pielines with
SqlTransforms in containerized notebook environment.
2. Before the change, pipelines with external transforms cannot be
executed if the SDK is running inside a docker container.
3. Even after the change, the docker container running the SDK needs to
set up Java and Docker (DooD or DinD) for xlang support.
4. The change does not take effect if the SDK is not running in a
notebook(ipython) environment.
5. The change also works even if the notebook environment running the
SDK is not in a docker container.
------------------------
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] KevinGG commented on a change in pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #13944:
URL: https://github.com/apache/beam/pull/13944#discussion_r575601177
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
'win32' falls back to the original logic.
The purpose is to only deal with linux-like system where we have existing features that need this. I could change this to sys.platform == 'linux' if we want to further limit the platform that need this change.
----------------------------------------------------------------
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] codecov[bot] commented on pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #13944:
URL: https://github.com/apache/beam/pull/13944#issuecomment-776320272
# [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=h1) Report
> Merging [#13944](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=desc) (51717c5) into [master](https://codecov.io/gh/apache/beam/commit/705649dc2f577e0fa4d8c766b89f7489d70acc60?el=desc) (705649d) will **increase** coverage by `0.00%`.
> The diff coverage is `16.66%`.
[](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13944 +/- ##
=======================================
Coverage 82.87% 82.87%
=======================================
Files 466 466
Lines 57613 57617 +4
=======================================
+ Hits 47745 47750 +5
+ Misses 9868 9867 -1
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...nners/portability/fn\_api\_runner/worker\_handlers.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3dvcmtlcl9oYW5kbGVycy5weQ==) | `79.61% <16.66%> (-0.39%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/source\_test\_utils.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vc291cmNlX3Rlc3RfdXRpbHMucHk=) | `88.73% <0.00%> (-1.36%)` | :arrow_down: |
| [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
| [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.70% <0.00%> (-0.13%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/direct/executor.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kaXJlY3QvZXhlY3V0b3IucHk=) | `96.82% <0.00%> (+0.52%)` | :arrow_up: |
| [sdks/python/apache\_beam/io/localfilesystem.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vbG9jYWxmaWxlc3lzdGVtLnB5) | `92.42% <0.00%> (+0.75%)` | :arrow_up: |
| [...ks/python/apache\_beam/runners/worker/data\_plane.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvZGF0YV9wbGFuZS5weQ==) | `91.31% <0.00%> (+1.79%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=footer). Last update [595c252...51717c5](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #13944:
URL: https://github.com/apache/beam/pull/13944#discussion_r575590762
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
I'm not sure this is always necessary when running inside a notebook. I was able to run SqlTransform in a notebook for our demo last fall: https://gist.github.com/TheNeuralBit/9c79d71cbc90a962e795b80ca54fa3c8#file-simpler-python-pipelines-demo-ipynb
It seems like this is necessary in your case because the IPython kernel is running inside a docker container, but that's not always true. In my case I was running the kernel directly on my desktop. Is there something else we can test here?
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
Why the special case for windows?
----------------------------------------------------------------
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] KevinGG commented on a change in pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #13944:
URL: https://github.com/apache/beam/pull/13944#discussion_r575601177
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
The purpose is to only deal with linux-like system, so I could change this to sys.platform == 'linux' if we want to further limit the platform that need this change.
----------------------------------------------------------------
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] codecov[bot] edited a comment on pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13944:
URL: https://github.com/apache/beam/pull/13944#issuecomment-776320272
# [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=h1) Report
> Merging [#13944](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=desc) (51717c5) into [master](https://codecov.io/gh/apache/beam/commit/705649dc2f577e0fa4d8c766b89f7489d70acc60?el=desc) (705649d) will **increase** coverage by `0.00%`.
> The diff coverage is `16.66%`.
[](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13944 +/- ##
=======================================
Coverage 82.87% 82.87%
=======================================
Files 466 466
Lines 57613 57617 +4
=======================================
+ Hits 47745 47750 +5
+ Misses 9868 9867 -1
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...nners/portability/fn\_api\_runner/worker\_handlers.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3dvcmtlcl9oYW5kbGVycy5weQ==) | `79.61% <16.66%> (-0.39%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/source\_test\_utils.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vc291cmNlX3Rlc3RfdXRpbHMucHk=) | `88.73% <0.00%> (-1.36%)` | :arrow_down: |
| [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
| [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.70% <0.00%> (-0.13%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/direct/executor.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kaXJlY3QvZXhlY3V0b3IucHk=) | `96.82% <0.00%> (+0.52%)` | :arrow_up: |
| [sdks/python/apache\_beam/io/localfilesystem.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vbG9jYWxmaWxlc3lzdGVtLnB5) | `92.42% <0.00%> (+0.75%)` | :arrow_up: |
| [...ks/python/apache\_beam/runners/worker/data\_plane.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvZGF0YV9wbGFuZS5weQ==) | `91.31% <0.00%> (+1.79%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=footer). Last update [595c252...3dae338](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit merged pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
TheNeuralBit merged pull request #13944:
URL: https://github.com/apache/beam/pull/13944
----------------------------------------------------------------
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] KevinGG commented on a change in pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #13944:
URL: https://github.com/apache/beam/pull/13944#discussion_r575602039
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
You're right, it's not necessary if the code is not running inside a container. But it still works even if the code is not inside a container.
The other thing we can check whether to enable this is through checking if the code is being executed in a docker container.
But it feels so clunky and fragile to check that: https://stackoverflow.com/questions/43878953/how-does-one-detect-if-one-is-running-within-a-docker-container-within-python/43879407
----------------------------------------------------------------
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] codecov[bot] edited a comment on pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13944:
URL: https://github.com/apache/beam/pull/13944#issuecomment-776320272
# [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=h1) Report
> Merging [#13944](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=desc) (3dae338) into [master](https://codecov.io/gh/apache/beam/commit/705649dc2f577e0fa4d8c766b89f7489d70acc60?el=desc) (705649d) will **decrease** coverage by `0.00%`.
> The diff coverage is `16.66%`.
[](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13944 +/- ##
==========================================
- Coverage 82.87% 82.86% -0.01%
==========================================
Files 466 466
Lines 57613 57617 +4
==========================================
- Hits 47745 47743 -2
- Misses 9868 9874 +6
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...nners/portability/fn\_api\_runner/worker\_handlers.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3dvcmtlcl9oYW5kbGVycy5weQ==) | `79.61% <16.66%> (-0.39%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/source\_test\_utils.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vc291cmNlX3Rlc3RfdXRpbHMucHk=) | `88.73% <0.00%> (-1.36%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/iobase.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vaW9iYXNlLnB5) | `84.55% <0.00%> (-0.27%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/common.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9jb21tb24ucHk=) | `88.87% <0.00%> (+0.14%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=footer). Last update [595c252...3dae338](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] edited a comment on pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13944:
URL: https://github.com/apache/beam/pull/13944#issuecomment-776320272
# [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=h1) Report
> Merging [#13944](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=desc) (3dae338) into [master](https://codecov.io/gh/apache/beam/commit/705649dc2f577e0fa4d8c766b89f7489d70acc60?el=desc) (705649d) will **decrease** coverage by `0.00%`.
> The diff coverage is `16.66%`.
[](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13944 +/- ##
==========================================
- Coverage 82.87% 82.86% -0.01%
==========================================
Files 466 466
Lines 57613 57617 +4
==========================================
- Hits 47745 47743 -2
- Misses 9868 9874 +6
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...nners/portability/fn\_api\_runner/worker\_handlers.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3dvcmtlcl9oYW5kbGVycy5weQ==) | `79.61% <16.66%> (-0.39%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/source\_test\_utils.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vc291cmNlX3Rlc3RfdXRpbHMucHk=) | `88.73% <0.00%> (-1.36%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/iobase.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vaW9iYXNlLnB5) | `84.55% <0.00%> (-0.27%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/common.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9jb21tb24ucHk=) | `88.87% <0.00%> (+0.14%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=footer). Last update [595c252...3dae338](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on a change in pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #13944:
URL: https://github.com/apache/beam/pull/13944#discussion_r578794944
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
There is some difference between accessing localhost and ip address: https://stackoverflow.com/questions/6938039/localhost-vs-real-ip-address.
For scenarios where localhost still works, we probably want to keep using localhost for better performance, depending how the user sets up their environment and runs pipelines. But for notebook environment, performance dip and Internet access are expected anyway, so we can safely enable 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] KevinGG commented on pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #13944:
URL: https://github.com/apache/beam/pull/13944#issuecomment-776313123
R: @rohdesamuel
R: @TheNeuralBit
Tested in 2 notebook environments:
- notebook running in a docker container:
- notebook running directly on a host machine:
----------------------------------------------------------------
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] codecov[bot] edited a comment on pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13944:
URL: https://github.com/apache/beam/pull/13944#issuecomment-776320272
# [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=h1) Report
> Merging [#13944](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=desc) (3dae338) into [master](https://codecov.io/gh/apache/beam/commit/ef685e6691c6e0ca4c34745cc8c4fc4550868305?el=desc) (ef685e6) will **increase** coverage by `0.00%`.
> The diff coverage is `16.66%`.
[](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13944 +/- ##
=======================================
Coverage 82.85% 82.86%
=======================================
Files 466 466
Lines 57613 57617 +4
=======================================
+ Hits 47738 47743 +5
+ Misses 9875 9874 -1
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...nners/portability/fn\_api\_runner/worker\_handlers.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3dvcmtlcl9oYW5kbGVycy5weQ==) | `79.61% <16.66%> (-0.39%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/common.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9jb21tb24ucHk=) | `88.87% <0.00%> (+0.14%)` | :arrow_up: |
| [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `87.50% <0.00%> (+1.04%)` | :arrow_up: |
| [sdks/python/apache\_beam/dataframe/schemas.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZGF0YWZyYW1lL3NjaGVtYXMucHk=) | `96.87% <0.00%> (+1.56%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=footer). Last update [595c252...0cb0460](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on a change in pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #13944:
URL: https://github.com/apache/beam/pull/13944#discussion_r578792202
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
Changed to check for 'linux' platform.
----------------------------------------------------------------
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] TheNeuralBit commented on a change in pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #13944:
URL: https://github.com/apache/beam/pull/13944#discussion_r578778768
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
Yeah I think we should check for linux if that's the goal, rather than relying on the fact that it's probably linux if it's not windows or mac.
----------------------------------------------------------------
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] codecov[bot] edited a comment on pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13944:
URL: https://github.com/apache/beam/pull/13944#issuecomment-776320272
# [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=h1) Report
> Merging [#13944](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=desc) (51717c5) into [master](https://codecov.io/gh/apache/beam/commit/705649dc2f577e0fa4d8c766b89f7489d70acc60?el=desc) (705649d) will **increase** coverage by `0.00%`.
> The diff coverage is `16.66%`.
[](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13944 +/- ##
=======================================
Coverage 82.87% 82.87%
=======================================
Files 466 466
Lines 57613 57617 +4
=======================================
+ Hits 47745 47750 +5
+ Misses 9868 9867 -1
```
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...nners/portability/fn\_api\_runner/worker\_handlers.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3dvcmtlcl9oYW5kbGVycy5weQ==) | `79.61% <16.66%> (-0.39%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/source\_test\_utils.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vc291cmNlX3Rlc3RfdXRpbHMucHk=) | `88.73% <0.00%> (-1.36%)` | :arrow_down: |
| [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
| [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.70% <0.00%> (-0.13%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/direct/executor.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kaXJlY3QvZXhlY3V0b3IucHk=) | `96.82% <0.00%> (+0.52%)` | :arrow_up: |
| [sdks/python/apache\_beam/io/localfilesystem.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vbG9jYWxmaWxlc3lzdGVtLnB5) | `92.42% <0.00%> (+0.75%)` | :arrow_up: |
| [...ks/python/apache\_beam/runners/worker/data\_plane.py](https://codecov.io/gh/apache/beam/pull/13944/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvZGF0YV9wbGFuZS5weQ==) | `91.31% <0.00%> (+1.79%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=footer). Last update [595c252...51717c5](https://codecov.io/gh/apache/beam/pull/13944?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #13944: [BEAM-10708] Support SqlTransform in container
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #13944:
URL: https://github.com/apache/beam/pull/13944#discussion_r578776686
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/worker_handlers.py
##########
@@ -737,11 +738,15 @@ def __init__(self,
def host_from_worker(self):
# type: () -> str
- if sys.platform == "darwin":
+ if sys.platform == 'darwin':
# See https://docs.docker.com/docker-for-mac/networking/
return 'host.docker.internal'
- else:
- return super(DockerSdkWorkerHandler, self).host_from_worker()
+ if sys.platform != 'win32' and is_in_notebook():
Review comment:
If it works whether we're in a container or not, should we just do this all the time?
----------------------------------------------------------------
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