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 2020/05/18 19:37:37 UTC
[GitHub] [beam] TheNeuralBit opened a new pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
TheNeuralBit opened a new pull request #11744:
URL: https://github.com/apache/beam/pull/11744
R: @aaltay
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/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](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/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.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.
----------------------------------------------------------------
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 #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit merged pull request #11744:
URL: https://github.com/apache/beam/pull/11744
----------------------------------------------------------------
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 pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-655192320
Looks like the CI failure is actually another flake due to BEAM-10006. It can be replicated reliably locally with
```
$ pytest apache_beam/runners/portability/fn_api_runner/fn_runner_test.py::FnApiRunnerTest::test_create_value_provider_pipeline_option apache_beam/runners/worker/sdk_worker_main_test.py
```
----------------------------------------------------------------
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 pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-631653792
Somehow this seems to introduce an additional flake in `SdkWorkerMainTest`. I can't replicate it locally... I suspect it only occurs when pipeline options get polluted from other tests.
----------------------------------------------------------------
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 #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #11744:
URL: https://github.com/apache/beam/pull/11744#discussion_r427592587
##########
File path: sdks/python/apache_beam/runners/portability/portable_runner.py
##########
@@ -164,10 +165,19 @@ def add_runner_options(parser):
all_options = self.options.get_all_options(
add_extra_args_fn=add_runner_options,
retain_unknown_options=self._retain_unknown_options)
+
# TODO: Define URNs for options.
# convert int values: https://issues.apache.org/jira/browse/BEAM-5509
+ def convert_pipeline_option_value(v):
+ if type(v) == int:
Review comment:
Oh the relevant jira is linked right there. It seems that this is really a workaround for a bug in json_format where ints and floats aren't treated differently. I can look into fixing that upstream so we can get rid of the special case here.
I don't think it would be good to call str(v) on everything
----------------------------------------------------------------
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] aaltay commented on a change in pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
aaltay commented on a change in pull request #11744:
URL: https://github.com/apache/beam/pull/11744#discussion_r427645484
##########
File path: sdks/python/apache_beam/runners/portability/portable_runner.py
##########
@@ -164,10 +165,19 @@ def add_runner_options(parser):
all_options = self.options.get_all_options(
add_extra_args_fn=add_runner_options,
retain_unknown_options=self._retain_unknown_options)
+
# TODO: Define URNs for options.
# convert int values: https://issues.apache.org/jira/browse/BEAM-5509
+ def convert_pipeline_option_value(v):
+ if type(v) == int:
Review comment:
Interesting. For this PR, could you move that comment closer to the if type(v) == int line.
> I can look into fixing that upstream so we can get rid of the special case here.
If you have time, maybe file a bug. I do not think this is a very high priority for us.
> I don't think it would be good to call str(v) on everything
I agree. I was trying to understand why are we doing 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] aaltay commented on pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-655209912
> Oh I see now. I tried to fix this issue by adding `drop_defaults=true` but we explicitly don't drop the default value for ValueProvider arguments:
>
> https://github.com/apache/beam/blob/1fbc55e7819187018c2cb3cb295eb84e6f348d8a/sdks/python/apache_beam/options/pipeline_options.py#L319-L320
>
> I'm not really sure why that is but it's been there for three years so I'm a little hesitant to remove it... I just pushed [982e087](https://github.com/apache/beam/commit/982e087fd09509277057b76991598cad3e6acf8e) that does this, let's see if it breaks anything in CI or Google tests.
>
> I'll check back in the morning, if it breaks anything (or if anyone tells me this is a bad idea), I can re-write the test to only assert on specific options.
I do not remember the reason for this. I am looking.
@tvalentyn - do you remember?
----------------------------------------------------------------
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] aaltay commented on pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-655211272
> > Oh I see now. I tried to fix this issue by adding `drop_defaults=true` but we explicitly don't drop the default value for ValueProvider arguments:
> > https://github.com/apache/beam/blob/1fbc55e7819187018c2cb3cb295eb84e6f348d8a/sdks/python/apache_beam/options/pipeline_options.py#L319-L320
> >
> > I'm not really sure why that is but it's been there for three years so I'm a little hesitant to remove it... I just pushed [982e087](https://github.com/apache/beam/commit/982e087fd09509277057b76991598cad3e6acf8e) that does this, let's see if it breaks anything in CI or Google tests.
> > I'll check back in the morning, if it breaks anything (or if anyone tells me this is a bad idea), I can re-write the test to only assert on specific options.
>
> I do not remember the reason for this. I am looking.
>
> @tvalentyn - do you remember?
I _believe_ default value of RuntimeValueProviders are actually user provided non-default values. (If I am reading this correctly: https://github.com/apache/beam/blob/1fbc55e7819187018c2cb3cb295eb84e6f348d8a/sdks/python/apache_beam/options/pipeline_options.py#L115)
----------------------------------------------------------------
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] aaltay commented on a change in pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
aaltay commented on a change in pull request #11744:
URL: https://github.com/apache/beam/pull/11744#discussion_r427565668
##########
File path: sdks/python/apache_beam/runners/portability/portable_runner.py
##########
@@ -164,10 +165,19 @@ def add_runner_options(parser):
all_options = self.options.get_all_options(
add_extra_args_fn=add_runner_options,
retain_unknown_options=self._retain_unknown_options)
+
# TODO: Define URNs for options.
# convert int values: https://issues.apache.org/jira/browse/BEAM-5509
+ def convert_pipeline_option_value(v):
+ if type(v) == int:
Review comment:
why there is a special case for int? Can we do str(v) for everything except for valueproviders?
----------------------------------------------------------------
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] aaltay commented on pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-631667834
> test_parse_pipeline_options
This test is a bit flawed. `expected_options.get_all_options()` could return all registered options. Test is assuming that no other option related tests has run.
The main flaw is still in our subclass based option discovery.
----------------------------------------------------------------
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 pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-631673799
Ah yeah I could change the test to just verify the options that are being set there (either by dropping default values in the get_all_options call, or by adding explicit asserts)
----------------------------------------------------------------
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 pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-655715080
Removing the ValueProvider line didn't actually break any Google tests, but I lost my resolve to remove it. It seems likely it would break some untested behavior, e.g. display_data uses drop_defaults=True:
https://github.com/apache/beam/blob/f65a18760a07a48c11fe4aff2a48a845df1f522d/sdks/python/apache_beam/options/pipeline_options.py#L328-L329
Instead I pushed d2d4ecb which makes the assertions in sdk_worker_main_test more specific. I think this is good to go assuming CI passses.
----------------------------------------------------------------
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] aaltay commented on pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-631658770
> Somehow this seems to introduce an additional flake in `SdkWorkerMainTest`. I can't replicate it locally... I suspect it only occurs when pipeline options get polluted from other tests.
What is the error?
----------------------------------------------------------------
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 #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #11744:
URL: https://github.com/apache/beam/pull/11744#discussion_r427586062
##########
File path: sdks/python/apache_beam/runners/portability/portable_runner.py
##########
@@ -164,10 +165,19 @@ def add_runner_options(parser):
all_options = self.options.get_all_options(
add_extra_args_fn=add_runner_options,
retain_unknown_options=self._retain_unknown_options)
+
# TODO: Define URNs for options.
# convert int values: https://issues.apache.org/jira/browse/BEAM-5509
+ def convert_pipeline_option_value(v):
+ if type(v) == int:
Review comment:
Good question. I was just maintaining the status quo and adding a special case for `ValueProvider`. Let me see if I can dig up why the str(v) is there for ints
----------------------------------------------------------------
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 pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-649012660
Run Python 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-655207778
Oh I see now. I tried to fix this issue by adding `drop_defaults=true` but we explicitly don't drop the default value for ValueProvider arguments: https://github.com/apache/beam/blob/1fbc55e7819187018c2cb3cb295eb84e6f348d8a/sdks/python/apache_beam/options/pipeline_options.py#L319-L320
I'm not really sure why that is but it's been there for three years so I'm a little hesitant to remove it... I just pushed 982e087 that does this, let's see if it breaks anything in CI or Google tests.
I'll check back in the morning, if it breaks anything (or if anyone tells me this is a bad idea), I can re-write the test to only assert on specific options.
----------------------------------------------------------------
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] aaltay commented on pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-655189985
----------------------------------------------------------------
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 pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-631630548
Run Python 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on a change in pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
aaltay commented on a change in pull request #11744:
URL: https://github.com/apache/beam/pull/11744#discussion_r427645484
##########
File path: sdks/python/apache_beam/runners/portability/portable_runner.py
##########
@@ -164,10 +165,19 @@ def add_runner_options(parser):
all_options = self.options.get_all_options(
add_extra_args_fn=add_runner_options,
retain_unknown_options=self._retain_unknown_options)
+
# TODO: Define URNs for options.
# convert int values: https://issues.apache.org/jira/browse/BEAM-5509
+ def convert_pipeline_option_value(v):
+ if type(v) == int:
Review comment:
Interesting. For this PR, could you move that comment closer to the if type(v) == int line.
> I can look into fixing that upstream so we can get rid of the special case here.
If you have time, maybe file a bug. I do not think this is a very high priority for us.
> I don't think it would be good to call str(v) on everything
I agree. I was trying to understand why are we doing 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] TheNeuralBit commented on pull request #11744: [BEAM-10007] Handle ValueProvider pipeline options in PortableRunner
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11744:
URL: https://github.com/apache/beam/pull/11744#issuecomment-631661434
```
self = <apache_beam.runners.worker.sdk_worker_main_test.SdkWorkerMainTest testMethod=test_parse_pipeline_options>
def test_parse_pipeline_options(self):
expected_options = PipelineOptions([])
expected_options.view_as(
SdkWorkerMainTest.MockOptions).m_m_option = ['beam_fn_api']
expected_options.view_as(
SdkWorkerMainTest.MockOptions).m_option = '/tmp/requirements.txt'
self.assertEqual(
expected_options.get_all_options(),
sdk_worker_main._parse_pipeline_options(
'{"options": {' + '"m_option": "/tmp/requirements.txt", ' +
> '"m_m_option":["beam_fn_api"]' + '}}').get_all_options())
E AssertionError: {'inf[2214 chars]7fd7c06bc278>, 'environment_config': None, 'in[616 chars]None} != {'inf[2214 chars]7fd7c10a35c0>, 'environment_config': None, 'in[616 chars]None}
E Diff is 3496 characters long. Set self.maxDiff to None to see it.
apache_beam/runners/worker/sdk_worker_main_test.py:70: AssertionError
```
I haven't looked into the test any further, but it looks like a memory address is getting pulled into the options and not matching.
----------------------------------------------------------------
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