[BEAM-5443] Pipeline option defaults for portable runner. #6512
Conversation
|
R: @angoenka |
|
The command line with these changes: |
sdks/python/apache_beam/pipeline.py
Outdated
| # (needs to occur prior to pipeline construction) | ||
| if self._options.view_as(StandardOptions).runner == 'PortableRunner': | ||
| self._options.view_as(DebugOptions).experiments = ( | ||
| self._options.view_as(DebugOptions).experiments or ['beam_fn_api']) |
There was a problem hiding this comment.
We should check and appendbeam_fn_api to experiments.
There was a problem hiding this comment.
The idea was to only touch experiments when the user did not set it. Do you think we should just always add beam_fn_api instead?
There was a problem hiding this comment.
I think it should be ok to add beam_fn_api always as without it the pipeline will fail.
And the behavior will be inconsistent where when no experiment is provided then the pipeline works but when when experiments is provided without explicit beam_fn_api, the pipeline fails.
|
|
||
| # portable runner specific default | ||
| if pipeline.options.view_as(SetupOptions).sdk_location == 'default': | ||
| pipeline.options.view_as(SetupOptions).sdk_location = 'container' |
There was a problem hiding this comment.
I wonder, shouldn't we change the interpretation of "default" in case of the portable runner to be container, instead of changing the value here?
There was a problem hiding this comment.
That would be cleaner, but stager.py may be used by dataflow or other modules that have a different interpretation of default. @angoenka ?
There was a problem hiding this comment.
For Dataflow, default means pypi.
Default can mean different thing based on the runner so translating default to container seems reasonable.
There was a problem hiding this comment.
If we can change the interpretation of default in the FlinkRunner to be container, I'd prefer that. It looks like changing this here could cause unexpected behavior.
There was a problem hiding this comment.
It is kind of strange to have that 'default' default value. Should it not be just None when there is no real default? Maybe we can change that in a follow-up PR.
There was a problem hiding this comment.
@mxm I don't think it can be deferred to the runner, the stager behavior depends on it. I would prefer that the stager does nothing unless a specific option was set (like pypi in dataflow case), but that should probably be a separate change. It would allow us to not even touch the value in portable runner.
There was a problem hiding this comment.
You're right, that doesn't work. I just find it odd to rewrite default -> container when the default could be container. The problem is that this option is used in different contexts (Portable Runner / Dataflow Runner).
Couldn't we alter the behavior of stager when the default value is used?
There was a problem hiding this comment.
tweise
force-pushed
the
BEAM-5443.portableDefaultOptions
branch
2 times, most recently
from
d03388a
to
4940b7c
Compare
tweise
force-pushed
the
BEAM-5443.portableDefaultOptions
branch
from
4940b7c
to
d3e6bab
Compare
Attempt to set portable runner specific defaults for experiments and sdk_location options.
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.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)