[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=390330&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-390330 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 21/Feb/20 00:04 Start Date: 21/Feb/20 00:04 Worklog Time Spent: 10m Work Description: angoenka commented on pull request #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919 **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: - [ ] [**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 | Apex | Dataflow | Flink | Gearpump | Samza | Spark --- | --- | --- | --- | --- | --- | --- | --- Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/l
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=390331&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-390331 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 21/Feb/20 00:06 Start Date: 21/Feb/20 00:06 Worklog Time Spent: 10m Work Description: angoenka commented on issue #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#issuecomment-589433667 R: @ananvay @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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 390331) Time Spent: 20m (was: 10m) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 20m > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=390335&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-390335 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 21/Feb/20 00:30 Start Date: 21/Feb/20 00:30 Worklog Time Spent: 10m Work Description: ananvay commented on issue #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#issuecomment-589440130 Thanks Ankur! LGTM. 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 390335) Time Spent: 0.5h (was: 20m) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 0.5h > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=391017&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-391017 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 22/Feb/20 01:50 Start Date: 22/Feb/20 01:50 Worklog Time Spent: 10m Work Description: tvalentyn commented on pull request #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#discussion_r382875122 ## File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py ## @@ -916,6 +919,9 @@ def _use_unified_worker(pipeline_options): debug_options = pipeline_options.view_as(DebugOptions) use_unified_worker_flag = 'use_unified_worker' + if debug_options.lookup_experiment(use_unified_worker_flag): Review comment: PTAL at this method - we need to remove some of the statements in 922-928. 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 391017) Time Spent: 40m (was: 0.5h) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Assignee: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 40m > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=391022&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-391022
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 22/Feb/20 01:58
Start Date: 22/Feb/20 01:58
Worklog Time Spent: 10m
Work Description: tvalentyn commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r382875832
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,11 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
+ # Don't add the default image overwrite if user overwrites or
Review comment:
I think it will be easier to read if we restructure this:
```
if (self.debug_options.lookup_experiment('runner_harness_container_image')
or
_use_unified_worker(self.debug_options)):
# Comment on WHY we don't want to set the override
pass
else:
<...set the override...>
```
WDYT?
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 391022)
Time Spent: 50m (was: 40m)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=391024&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-391024
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 22/Feb/20 02:00
Start Date: 22/Feb/20 02:00
Worklog Time Spent: 10m
Work Description: tvalentyn commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r382876017
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,11 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
+ # Don't add the default image overwrite if user overwrites or
Review comment:
Also, should we be passing pipeline_options instead of debug options?
Perhaps it works either way but passing pipeline options would be cleaner since
that's what the signature of _use_unified_worker() expects.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 391024)
Time Spent: 1h (was: 50m)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 1h
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=391025&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-391025 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 22/Feb/20 02:01 Start Date: 22/Feb/20 02:01 Worklog Time Spent: 10m Work Description: tvalentyn commented on pull request #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#discussion_r382876179 ## File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py ## @@ -916,6 +919,9 @@ def _use_unified_worker(pipeline_options): debug_options = pipeline_options.view_as(DebugOptions) use_unified_worker_flag = 'use_unified_worker' + if debug_options.lookup_experiment(use_unified_worker_flag): Review comment: Do we need 922-924? 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 391025) Time Spent: 1h 10m (was: 1h) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Assignee: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 1h 10m > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=391026&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-391026 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 22/Feb/20 02:03 Start Date: 22/Feb/20 02:03 Worklog Time Spent: 10m Work Description: tvalentyn commented on pull request #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#discussion_r382876346 ## File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py ## @@ -916,6 +919,9 @@ def _use_unified_worker(pipeline_options): debug_options = pipeline_options.view_as(DebugOptions) use_unified_worker_flag = 'use_unified_worker' + if debug_options.lookup_experiment(use_unified_worker_flag): Review comment: nvm, reading this again, looks like this logic is intenional 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 391026) Time Spent: 1h 20m (was: 1h 10m) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Assignee: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 1h 20m > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=391028&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-391028 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 22/Feb/20 02:20 Start Date: 22/Feb/20 02:20 Worklog Time Spent: 10m Work Description: tvalentyn commented on pull request #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#discussion_r382877617 ## File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py ## @@ -916,6 +919,9 @@ def _use_unified_worker(pipeline_options): debug_options = pipeline_options.view_as(DebugOptions) use_unified_worker_flag = 'use_unified_worker' + if debug_options.lookup_experiment(use_unified_worker_flag): Review comment: Ok, I think I understand the intent but I think it is confusing - can we move lines 925-926 to dataflow_runner.py, see: https://github.com/apache/beam/blob/1117508136ac5d0d66141c0e8f1c0830fb223a3a/sdks/python/apache_beam/runners/dataflow/dataflow_runner.py#L480, and replace 920-928 with line 928? 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 391028) Time Spent: 1.5h (was: 1h 20m) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Assignee: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 1.5h > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392073&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392073 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 24/Feb/20 20:29 Start Date: 24/Feb/20 20:29 Worklog Time Spent: 10m Work Description: angoenka commented on pull request #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#discussion_r383492636 ## File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py ## @@ -916,6 +919,9 @@ def _use_unified_worker(pipeline_options): debug_options = pipeline_options.view_as(DebugOptions) use_unified_worker_flag = 'use_unified_worker' + if debug_options.lookup_experiment(use_unified_worker_flag): Review comment: We can move 925-926 to dataflow_runner.py but then i feel the logic to determine unified_worker usage will be more staggered. I think it will be better to keep the interpretation of `use_unified_worker` and `use_runner_v2` at a single place. For now, we support both flag but I think we will transition to `use_runner_v2` over time (not sure by when). 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 392073) Time Spent: 1h 40m (was: 1.5h) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Assignee: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 1h 40m > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392074&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392074
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 24/Feb/20 20:29
Start Date: 24/Feb/20 20:29
Worklog Time Spent: 10m
Work Description: angoenka commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r383496637
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,11 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
+ # Don't add the default image overwrite if user overwrites or
Review comment:
> I think it will be easier to read if we restructure this:
>
> ```
> if (self.debug_options.lookup_experiment('runner_harness_container_image')
or
> _use_unified_worker(self.debug_options)):
># Comment on WHY we don't want to set the override
>pass
> else:
><...set the override...>
> ```
>
> WDYT?
I think it makes sense. Will update it.
> Also, should we be passing pipeline_options instead of debug options?
Perhaps it works either way but passing pipeline options would be cleaner since
that's what the signature of _use_unified_worker() expects.
I think the code is structured in a way that a `self.debug_options` is
referenced every where and the reference to it can be different from `options`
passed in the method argument. So I think it will need a more thoughtful
refactoring to move to change it to options in the right manner. Which I think
we can differ.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392074)
Time Spent: 1h 40m (was: 1.5h)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392082&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392082
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 24/Feb/20 20:49
Start Date: 24/Feb/20 20:49
Worklog Time Spent: 10m
Work Description: angoenka commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r383496637
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,11 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
+ # Don't add the default image overwrite if user overwrites or
Review comment:
> I think it will be easier to read if we restructure this:
>
> ```
> if (self.debug_options.lookup_experiment('runner_harness_container_image')
or
> _use_unified_worker(self.debug_options)):
># Comment on WHY we don't want to set the override
>pass
> else:
><...set the override...>
> ```
>
> WDYT?
I think it makes sense. Will update it.
> Also, should we be passing pipeline_options instead of debug options?
Perhaps it works either way but passing pipeline options would be cleaner since
that's what the signature of _use_unified_worker() expects.
I think the code is structured in a way that a `self.debug_options` is
referenced every where and the reference to it can be different from `options`
passed in the method argument. So I think it will need a more thoughtful
refactoring to move to change it to options in the right manner. Which I think
we can differ.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392082)
Time Spent: 1h 50m (was: 1h 40m)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392092&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392092
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 24/Feb/20 21:16
Start Date: 24/Feb/20 21:16
Worklog Time Spent: 10m
Work Description: tvalentyn commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r383516953
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,13 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
- if not self.debug_options.lookup_experiment(
- 'runner_harness_container_image'):
+ if self.debug_options.lookup_experiment(
Review comment:
Few thoughts here
1. I think we cannot instantiate `DebugOptions()` in `self.debug_options =
self.debug_options or DebugOptions()`. We should create a view of existing
`options`. All views of pipeline options should be views of the same
underlying object, and I think that line will break it. Do you know why we
added it?
2. For the purpose of this PR it should be sufficient to pass `options` into
`_use_unified_worker()` in line 190.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392092)
Time Spent: 2h (was: 1h 50m)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 2h
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392093&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392093
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 24/Feb/20 21:16
Start Date: 24/Feb/20 21:16
Worklog Time Spent: 10m
Work Description: tvalentyn commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r383518791
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,13 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
- if not self.debug_options.lookup_experiment(
- 'runner_harness_container_image'):
+ if self.debug_options.lookup_experiment(
+ 'runner_harness_container_image') or _use_unified_worker(
+ self.debug_options):
+# Don't add the default image overwrite if user overwrites or
Review comment:
This comment repeats the logic; I thought you might want to explain why we
don't want to pin the image for UW.
Consider: "In case of using UW, harness container should be defined by DF
service unless explicitly overridden."
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392093)
Time Spent: 2h (was: 1h 50m)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 2h
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392091&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392091 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 24/Feb/20 21:16 Start Date: 24/Feb/20 21:16 Worklog Time Spent: 10m Work Description: tvalentyn commented on pull request #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#discussion_r383516522 ## File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py ## @@ -916,6 +919,9 @@ def _use_unified_worker(pipeline_options): debug_options = pipeline_options.view_as(DebugOptions) use_unified_worker_flag = 'use_unified_worker' + if debug_options.lookup_experiment(use_unified_worker_flag): Review comment: I think that it would be cleaner to not modify experiments in `_use_unified_worker`, since the name of this method does not suggest that it modifies the state, and it would also be easier to follow the logic. Also, if Dataflow runner relies on the fact that "use_runner_v2" experiment is set, it might be better to set it explicitly at some point, instead of relying on the fact that "_use_unified_worker" will be called. Leaving this up to you. 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 392091) Time Spent: 2h (was: 1h 50m) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Assignee: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 2h > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392094&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392094
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 24/Feb/20 21:17
Start Date: 24/Feb/20 21:17
Worklog Time Spent: 10m
Work Description: tvalentyn commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r383516953
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,13 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
- if not self.debug_options.lookup_experiment(
- 'runner_harness_container_image'):
+ if self.debug_options.lookup_experiment(
Review comment:
Few thoughts here
1. I think we cannot instantiate `DebugOptions()` in `self.debug_options =
self.debug_options or DebugOptions()`. We should create a view of existing
`options`. All views of pipeline options should be views of the same
underlying object, and I think that line can break this invariant if we follow
the `or` branch. Do you know why we added it?
2. For the purpose of this PR it should be sufficient to pass `options` into
`_use_unified_worker()` in line 190.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392094)
Time Spent: 2h 10m (was: 2h)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392104&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392104
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 24/Feb/20 21:32
Start Date: 24/Feb/20 21:32
Worklog Time Spent: 10m
Work Description: angoenka commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r383525784
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,13 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
- if not self.debug_options.lookup_experiment(
- 'runner_harness_container_image'):
+ if self.debug_options.lookup_experiment(
+ 'runner_harness_container_image') or _use_unified_worker(
+ self.debug_options):
+# Don't add the default image overwrite if user overwrites or
Review comment:
Updates the comment.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392104)
Time Spent: 2h 20m (was: 2h 10m)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 2h 20m
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[
https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392105&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392105
]
ASF GitHub Bot logged work on BEAM-9347:
Author: ASF GitHub Bot
Created on: 24/Feb/20 21:32
Start Date: 24/Feb/20 21:32
Worklog Time Spent: 10m
Work Description: angoenka commented on pull request #10919: [BEAM-9347]
Don't overwrite default runner harness for unified worker
URL: https://github.com/apache/beam/pull/10919#discussion_r383522629
##
File path: sdks/python/apache_beam/runners/dataflow/internal/apiclient.py
##
@@ -186,8 +186,13 @@ def __init__(self, packages, options,
environment_version, pipeline_url):
if job_type.startswith('FNAPI_'):
self.debug_options = self.debug_options or DebugOptions()
self.debug_options.experiments = self.debug_options.experiments or []
- if not self.debug_options.lookup_experiment(
- 'runner_harness_container_image'):
+ if self.debug_options.lookup_experiment(
Review comment:
I agree. I think I added it because in certain case it was passed in test. I
might be wrong though.
Makes sense. I will update the PR with both these suggestions.
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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392105)
Time Spent: 2.5h (was: 2h 20m)
> Remove default image for Unified Worker
> ---
>
> Key: BEAM-9347
> URL: https://issues.apache.org/jira/browse/BEAM-9347
> Project: Beam
> Issue Type: Test
> Components: runner-dataflow
>Reporter: Ankur Goenka
>Assignee: Ankur Goenka
>Priority: Major
> Fix For: 2.20.0
>
> Time Spent: 2.5h
> Remaining Estimate: 0h
>
> The runner will choose the Runner Harness image for UW so we don't need to
> overwrite the image in default behavior.
> Also, this will help us distinguish between user requested overwrites for the
> default overwrites(which is not used).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392132&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392132 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 24/Feb/20 22:35 Start Date: 24/Feb/20 22:35 Worklog Time Spent: 10m Work Description: angoenka commented on pull request #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 392132) Time Spent: 2h 40m (was: 2.5h) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Assignee: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 2h 40m > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (BEAM-9347) Remove default image for Unified Worker
[ https://issues.apache.org/jira/browse/BEAM-9347?focusedWorklogId=392133&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392133 ] ASF GitHub Bot logged work on BEAM-9347: Author: ASF GitHub Bot Created on: 24/Feb/20 22:35 Start Date: 24/Feb/20 22:35 Worklog Time Spent: 10m Work Description: angoenka commented on issue #10919: [BEAM-9347] Don't overwrite default runner harness for unified worker URL: https://github.com/apache/beam/pull/10919#issuecomment-590584036 Thanks @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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 392133) Time Spent: 2h 50m (was: 2h 40m) > Remove default image for Unified Worker > --- > > Key: BEAM-9347 > URL: https://issues.apache.org/jira/browse/BEAM-9347 > Project: Beam > Issue Type: Test > Components: runner-dataflow >Reporter: Ankur Goenka >Assignee: Ankur Goenka >Priority: Major > Fix For: 2.20.0 > > Time Spent: 2h 50m > Remaining Estimate: 0h > > The runner will choose the Runner Harness image for UW so we don't need to > overwrite the image in default behavior. > Also, this will help us distinguish between user requested overwrites for the > default overwrites(which is not used). -- This message was sent by Atlassian Jira (v8.3.4#803005)
