You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "shunping (via GitHub)" <gi...@apache.org> on 2025/04/29 01:16:59 UTC
[PR] Skip deep copying runner instance in pipeline options. [beam]
shunping opened a new pull request, #34775:
URL: https://github.com/apache/beam/pull/34775
Fix internal test failures caused by #34723.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #34775:
URL: https://github.com/apache/beam/pull/34775#discussion_r2066362826
##########
sdks/python/apache_beam/pipeline.py:
##########
@@ -173,8 +173,13 @@ def __init__(
if options is not None:
if isinstance(options, PipelineOptions):
# Make a deep copy of options since they could be overwritten in later
- # steps.
+ # steps. However, the 'runner' object within 'options' is excluded from
+ # the deep copy (it is shallow copied) due to potential issues with deep
+ # copying specific runner instances, such as FlumeRunner.
+ saved_runner = options.view_as(StandardOptions).runner
+ options.view_as(StandardOptions).runner = None
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2838320805
Run Yaml_Xlang_Direct PreCommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2838959341
Is it in general safe to copy Pipeline Options? Can you think of any other cases where it might cause issues like copying runner>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2839142200
Run Python_Integration PreCommit 3.9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "jrmccluskey (via GitHub)" <gi...@apache.org>.
jrmccluskey merged PR #34775:
URL: https://github.com/apache/beam/pull/34775
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2838984345
> Is it in general safe to copy Pipeline Options? Can you think of any other cases where it might cause issues like copying runner>
I think it should generally work. There are also other places that do deep copy of options:
- https://github.com/apache/beam/blob/95965ffd5c6670e3eac1d4dbad9a09a71a5dc880/sdks/python/apache_beam/runners/portability/portable_runner.py#L399
- https://github.com/apache/beam/blob/95965ffd5c6670e3eac1d4dbad9a09a71a5dc880/sdks/python/apache_beam/runners/portability/expansion_service.py#L58
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm commented on code in PR #34775:
URL: https://github.com/apache/beam/pull/34775#discussion_r2066300418
##########
sdks/python/apache_beam/pipeline.py:
##########
@@ -173,8 +173,13 @@ def __init__(
if options is not None:
if isinstance(options, PipelineOptions):
# Make a deep copy of options since they could be overwritten in later
- # steps.
+ # steps. However, the 'runner' object within 'options' is excluded from
+ # the deep copy (it is shallow copied) due to potential issues with deep
+ # copying specific runner instances, such as FlumeRunner.
+ saved_runner = options.view_as(StandardOptions).runner
+ options.view_as(StandardOptions).runner = None
Review Comment:
Is it ok to remove runner from the original options? Should it not be added back?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2839073103
What do you think about adding a method on PipelineOptions.copy that does the copying logic instead of doing it in Pipeline?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2839096935
> What do you think about adding a method on PipelineOptions.copy that does the copying logic instead of doing it in Pipeline?
I think that is a good idea, but I would say we may refactor that after the coming release cut (in 1 day) so that we can have more time to test and fix any breakage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2837233838
Run Python_Integration PreCommit 3.9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #34775:
URL: https://github.com/apache/beam/pull/34775#discussion_r2066330652
##########
sdks/python/apache_beam/pipeline.py:
##########
@@ -173,8 +173,13 @@ def __init__(
if options is not None:
if isinstance(options, PipelineOptions):
# Make a deep copy of options since they could be overwritten in later
- # steps.
+ # steps. However, the 'runner' object within 'options' is excluded from
+ # the deep copy (it is shallow copied) due to potential issues with deep
+ # copying specific runner instances, such as FlumeRunner.
+ saved_runner = options.view_as(StandardOptions).runner
+ options.view_as(StandardOptions).runner = None
Review Comment:
You are right. I must have accidentally removed that line during code refactoring.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2837332828
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @claudevdm for label python.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2838978377
R: @damccorm for final approval
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Skip deep copying runner instance in pipeline options. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on PR #34775:
URL: https://github.com/apache/beam/pull/34775#issuecomment-2838766645
The test failure of YAML Xlang is unrelated to the change here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org