Skip to content

[BEAM-5462] get rid of <pipeline>.options deprecation warnings in tests#6930

Merged
chamikaramj merged 1 commit intoapache:masterfrom
ihji:p_options_depr
Dec 4, 2018
Merged

[BEAM-5462] get rid of <pipeline>.options deprecation warnings in tests#6930
chamikaramj merged 1 commit intoapache:masterfrom
ihji:p_options_depr

Conversation

@ihji
Copy link
Contributor

@ihji ihji commented Nov 2, 2018

Add an additional parameter options to PipelineRunner.run_pipeline(self, pipeline) and change the usage of pipeline.options() and pipeline._options to the parameter options.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • 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.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@ihji
Copy link
Contributor Author

ihji commented Nov 2, 2018

@udim Looks like it's not easy to fix this issue without public API signature changes and this is a proposal for it. Any idea / feedback is welcomed.

@ihji
Copy link
Contributor Author

ihji commented Nov 14, 2018

@robertwb Any comment?

@ihji
Copy link
Contributor Author

ihji commented Nov 14, 2018

run python postcommit

Copy link
Member

@udim udim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. @ccy, @robertwb do you see any possible issues? Is this the intended direction?

p.options.view_as(StandardOptions).streaming = True
options = PipelineOptions([])
p = TestPipeline(options=options)
options.view_as(StandardOptions).streaming = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this works, but setting streaming should happen before passing options to TestPipeline, since TestPipeline might make a copy of options, already evaluated streaming in the constructor, etc.

@robertwb
Copy link
Contributor

I'm not sure we want to bind pipelines to a runner, e.g. one could imagine using the same runner to multiple pipelines possibly with different options. Instead, the options should be passed into run_pipeline.

@ihji
Copy link
Contributor Author

ihji commented Nov 20, 2018

Then it would be better to wait until 3.0 release and add an additional parameter to run_pipeline. How about just removing deprecated annotation for now? There's no way to avoid that deprecation warning with the current method structure.

@robertwb
Copy link
Contributor

If we remove the annotation now then it will be all the harder to remove it later.

run_pipeline is not considered part of the immutable public API, it's a runner implementation detail. This change can be made now.

@ihji
Copy link
Contributor Author

ihji commented Nov 26, 2018

new implementation #7132

@chamikaramj
Copy link
Contributor

Can you update the existing PR instead of opening a new one so that review can be continued at the same location ?

If above is not possible for some reason please close this PR instead of the new one.

@ihji
Copy link
Contributor Author

ihji commented Dec 3, 2018

@chamikaramj I'll close the new one and update this.

adding a new parameter `options` to `runner.run_pipeline` method
@ihji
Copy link
Contributor Author

ihji commented Dec 4, 2018

run python postcommit

@udim
Copy link
Member

udim commented Dec 4, 2018

Run Python Dataflow ValidatesRunner
Run Python Flink ValidatesRunner

@udim
Copy link
Member

udim commented Dec 4, 2018

Run Python Dataflow ValidatesRunner

@udim
Copy link
Member

udim commented Dec 4, 2018

Run Python Flink ValidatesRunner

@chamikaramj
Copy link
Contributor

Thanks Heejong.

Looks like Robert's above comment has been addressed as well. I'll go ahead and merge.

@chamikaramj chamikaramj merged commit 95d0ac5 into apache:master Dec 4, 2018
@ihji ihji deleted the p_options_depr branch December 10, 2018 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants