[BEAM-802] Add ValueProvider class for FileBasedSource I/O Transforms by mariapython · Pull Request #1945 · apache/beam

mariapython · 2017-02-08T05:28:01Z

Add ValueProvider class.
Derive StaticValueProvider and RuntimeValueProvider from ValueProvider.
Derive ValueProviderArgumentParser from argparse.ArgumentParser as API for the template user.
Modify FileBasedSource I/O transforms to accept objects of type ValueProvider.
Modify display_data.
Handle serialization / deserialization.

coveralls · 2017-02-08T05:59:59Z

Coverage decreased (-0.01%) to 69.595% when pulling f0dab8f on mariapython:ppp_inmaster into 8c1a577 on apache:master.

asfbot · 2017-02-08T06:00:29Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7185/
--none--

mariapython · 2017-02-08T19:59:21Z

R: @robertwb

coveralls · 2017-02-21T18:35:35Z

Coverage increased (+0.005%) to 69.172% when pulling aaa6963 on mariapython:ppp_inmaster into a991c9d on apache:master.

asfbot · 2017-02-21T18:36:41Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7633/
--none--

coveralls · 2017-02-22T06:58:56Z

Coverage decreased (-0.02%) to 69.284% when pulling 9560065 on mariapython:ppp_inmaster into 0d639e6 on apache:master.

asfbot · 2017-02-22T06:59:27Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7687/
--none--

aaltay

Thank you @mariapython. I added my comments, let's start iterating on those.

aaltay · 2017-02-23T18:50:54Z

sdks/python/apache_beam/internal/google_cloud_platform/json_value.py

+  elif isinstance(obj, ValueProvider):
+    if obj.is_accessible():
+      return to_json_value(obj.get())
+    return extra_types.JsonValue(is_null=True)


@sammcveety

Is this the correct serialization for runtime value providers? (pipeline options will have a "key": None serialization)

I mimicked Java's behavior:
https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ValueProvider.java#L276

aaltay · 2017-02-23T18:53:04Z

sdks/python/apache_beam/io/filebasedsource.py

 from apache_beam.transforms.display import DisplayDataItem
+from apache_beam.utils.value_provider import ValueProvider
+from apache_beam.utils.value_provider import StaticValueProvider
+from apache_beam.utils.value_provider import RuntimeValueProvider


Does RuntimeValueProvider need to be exposed as a public interface. ValueProvider and StaticValueProvider could be the only things that is public outside the value_provider module and maybe a few places.

Oops, that was a slip--fixed.

aaltay · 2017-02-23T18:54:00Z

sdks/python/apache_beam/io/filebasedsource.py

      # We can't split compressed files efficiently so turn off splitting.
      self._splittable = False
-    if validate:
+    if validate and not isinstance(file_pattern, RuntimeValueProvider):


Could this be if validate and not file_pattern.is_accessible()? Is there a reason to check its type?

aaltay · 2017-02-23T18:56:37Z

sdks/python/apache_beam/io/filebasedsource.py


  def _get_concat_source(self):
    if self._concat_source is None:
+      if not self._pattern.is_accessible():


@robertwb

Should we add a get_or_error method to ValueProvider?. It is a common pattern to check is_accessible and raising RuntimeError.

aaltay · 2017-02-23T19:01:35Z

sdks/python/apache_beam/io/fileio.py

+    if not self.file_path_prefix.is_accessible():
+      raise RuntimeError('%s not accessible' % self.file_path_prefix)
+    file_path_prefix = self.file_path_prefix.get()
+    file_name_suffix = self.file_name_suffix.get()


Do you need to check if this is_accessible?

Forgot to pop a stash--done

aaltay · 2017-02-23T19:55:52Z

sdks/python/apache_beam/utils/value_provider.py

+    )
+    value = (
+        pipeline_option.get()
+        if isinstance(pipeline_option, StaticValueProvider)


why not use is_accessible instead of type checking?

Modeled after Java's.
https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ValueProvider.java#L234

aaltay · 2017-02-23T19:59:22Z

sdks/python/apache_beam/utils/value_provider.py

+    pipeline_options_dict = RuntimeValueProvider.pipeline_options_dict
+    if pipeline_options_dict is None:
+      raise RuntimeError('%s.get() not called from a runtime context' %self)
+    pipeline_option = (


What is the reason here for recreating an options instance? Can we read the value out of pipeline_options_dict?

This was modeled after Java's.

aaltay · 2017-02-23T19:59:46Z

sdks/python/apache_beam/utils/value_provider.py

+
+  @classmethod
+  def set_runtime_options(cls, pipeline_options):
+    RuntimeValueProvider.pipeline_options_dict = pipeline_options


maybe assert that RuntimeValueProvider.pipeline_options_dict is None

aaltay · 2017-02-23T20:00:47Z

sdks/python/apache_beam/utils/value_provider_test.py

+            type=int,
+            help='This flag is a value provider')
+
+    # options = UserOptions(['--vp_arg', 'hola'])


remove commented out code.

aaltay · 2017-02-23T20:01:45Z

sdks/python/apache_beam/utils/value_provider_test.py

+    assert options.vp_arg3.is_accessible() is True
+    assert options.vp_arg3.get() == '123'
+    assert options.vp_arg4.is_accessible() is True
+    assert options.vp_arg4.get() is None


Should not this fail? It is of RuntimeValueProvider and has no default.

RuntimeValueProvider objects with no default and no value injected don't fail.
In Java: https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ValueProvider.java#L191

coveralls · 2017-02-27T03:45:05Z

Coverage decreased (-0.01%) to 69.309% when pulling 9aff2a5 on mariapython:ppp_inmaster into 4bba380 on apache:master.

asfbot · 2017-02-27T03:45:36Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7869/
--none--

asfbot · 2017-03-01T07:08:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8003/

Build result: FAILURE

[...truncated 1.55 MB...] at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoExecutionException: Command execution failed. at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:302) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 moreCaused by: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) at org.apache.commons.exec.DefaultExecutor.executeInternal(DefaultExecutor.java:404) at org.apache.commons.exec.DefaultExecutor.execute(DefaultExecutor.java:166) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:764) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:711) at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:289) ... 33 more2017-03-01T07:08:47.327 [ERROR] 2017-03-01T07:08:47.327 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-01T07:08:47.327 [ERROR] 2017-03-01T07:08:47.327 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-01T07:08:47.327 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-03-01T07:08:47.328 [ERROR] 2017-03-01T07:08:47.328 [ERROR] After correcting the problems, you can resume the build with the command2017-03-01T07:08:47.328 [ERROR] mvn -rf :beam-sdks-pythonchannel stoppedSetting status of 97a3b53 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8003/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

coveralls · 2017-03-01T22:37:22Z

Coverage increased (+0.003%) to 69.887% when pulling 2916651 on mariapython:ppp_inmaster into b2428ea on apache:master.

asfbot · 2017-03-01T22:38:23Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8029/
--none--

coveralls · 2017-03-03T17:55:04Z

Coverage decreased (-0.01%) to 69.881% when pulling 9bdd605 on mariapython:ppp_inmaster into 1783819 on apache:master.

asfbot · 2017-03-03T17:55:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8078/
--none--

aaltay · 2017-03-03T18:55:36Z

sdks/python/apache_beam/utils/pipeline_options.py

+  """
+  def add_value_provider_argument(self, *args, **kwargs):
+    # Extract the option name from positional argument ['pos_arg']
+    if args[0][0] != '-':


There are two assumptions here. Could you assert those for a better error message.

args has at least length of 1 and args[0] exists.

and args[0] is indexable, like args[0] is a string.

Both are enforced by argparse's add_argument. i.e. CLI arguments are strings and an argument always requires a name. If you try to add an argument without a name, you get:
TypeError: _get_positional_kwargs() takes exactly 2 arguments (1 given)

Since the call to add_argument happens later, I have placed one assert to check for args to exist and have at least one element of size one. There is more I could add, but I don't want to obscure the code by doing heavy type checking here, since add_argument will take care of it a few lines below.

aaltay · 2017-03-03T18:56:04Z

sdks/python/apache_beam/utils/pipeline_options.py

+      option_name = args[0]
+      if kwargs.get('nargs') is None:  # make them optionally templated
+        kwargs['nargs'] = '?'
+    # or keyword arguments like [--kw_arg, -k, -w] or [--kw-arg]


Move this comment inside the else block

OK. The reason why I usually don't do it is because rigorously, that would make me have an extra comment line:

# Extract the option name if args[0][0] != '-': # from positional argument ['pos_arg'] option_name = args[0] if kwargs.get('nargs') is None: # make them optionally templated kwargs['nargs'] = '?' else: # or keyword arguments like [--kw_arg, -k, -w] or [--kw-arg] option_name = [i.replace('--', '') for i in args if i[:2] == '--'][0]

aaltay · 2017-03-03T19:05:58Z

sdks/python/apache_beam/utils/value_provider.py

+    )
+
+
+def check_accessible(fields):


Add a comment about what is this and how users could use it. It will have utility to developers building components.

Done long ago, although forgot to respond, it seems.

aaltay · 2017-03-03T23:32:05Z

sdks/python/apache_beam/utils/value_provider.py

+    if pipeline_options_dict is None:
+      raise RuntimeError('%s.get() not called from a runtime context' %self)
+    pipeline_option = (
+        self.pipeline_options_subclass.from_dictionary(pipeline_options_dict)


PipelineOptions.from_dictionary(pipeline_options_dict).view_as(pipeline_options_subclass).get(self.option_name)

PipelineOptions.from_dictionary(pipeline_options_dict).view_as(self.pipeline_options_subclass) would be OK, but not .get(...)

coveralls · 2017-03-06T08:31:21Z

Coverage remained the same at 70.085% when pulling 1ff44d5 on mariapython:ppp_inmaster into 2f96bc3 on apache:master.

asfbot · 2017-03-06T08:33:01Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8132/
--none--

coveralls · 2017-03-08T08:00:10Z

Coverage increased (+0.007%) to 70.051% when pulling c5569ca on mariapython:ppp_inmaster into 690e1e2 on apache:master.

coveralls · 2017-03-08T08:01:54Z

Coverage remained the same at 70.044% when pulling c5569ca on mariapython:ppp_inmaster into 690e1e2 on apache:master.

asfbot · 2017-03-08T08:06:15Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8200/

Failed Tests: 2

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 2

--none--

asfbot · 2017-03-08T08:07:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8201/
--none--

aaltay · 2017-03-08T18:50:26Z

sdks/python/apache_beam/examples/wordcount.py

+          dest='output',
+          required=True,
+          help='Output file to write results to.')
+  pipeline_options = UserOptions(argv)


Our canonical form is pipeline_options = PipelineOptions(argv). Could you convert to that form? It should work exactly the same.

If you mean this:
pipeline_options = PipelineOptions(argv).view_as(UserOptions)
Yes, it works, but I found it less readable.

It is possible for a user code to define multiple options. We want them to use:

pipeline_options = PipelineOptions(argv) user_options = pipeline_options.view_as(UserOption)

This is the standard way for the existing examples and documentation.

OK, I can then use those two lines, that is no problem

aaltay · 2017-03-08T19:18:01Z

sdks/python/apache_beam/examples/wordcount.py

-                      required=True,
-                      help='Output file to write results to.')
-  known_args, pipeline_args = parser.parse_known_args(argv)
+  class UserOptions(PipelineOptions):


We used to have a problem with defining options here (See: #1889 (comment))

Other tests started to fail because newly defined options stay in scope affected other tests. Is it resolved? Or this is not happening any more because the options are defaulting to ValueProviders?

I haven't seen that happening, as long as there is no call to get_all_options()

aaltay · 2017-03-08T19:21:23Z

sdks/python/apache_beam/utils/value_provider.py

-      for vp in [getattr(self, field) for field in fields]:
-        if not vp.is_accessible():
-          raise RuntimeError('%s not accessible' % vp)
+      for obj in [getattr(self, vp) for vp in value_provider_list]:


Could you use something other than obj. This is representing an option not just an object.

aaltay · 2017-03-08T19:54:45Z

sdks/python/apache_beam/utils/value_provider_test.py

+    self.assertEqual(options.vp_pos_arg.get(), 'abc')
+
+  def test_runtime_value_provider_positional_argument(self):
+    class UserOptions(PipelineOptions):


Change UserOptions to a more unique name, they potentially cause name conflicts with other tests.

aaltay · 2017-03-08T19:55:45Z

sdks/python/apache_beam/utils/value_provider_test.py

    self.assertFalse(options.vp_arg3.is_accessible())
    self.assertFalse(options.vp_arg4.is_accessible())
-    self.assertFalse(options.vp_pos_arg.is_accessible())
+    # self.assertFalse(options.vp_pos_arg.is_accessible())


Remove commented out code. And why this is accessible when other vps are not?

Done. It is accessible because a positional argument value has been given as option: '1.2'

aaltay · 2017-03-08T19:56:30Z

sdks/python/apache_beam/examples/wordcount.py

-                      required=True,
-                      help='Output file to write results to.')
-  known_args, pipeline_args = parser.parse_known_args(argv)
+  class UserOptions(PipelineOptions):


WordCountOptions maybe?

I do not see the change?

I wanted to push all the changes at once, and I am working on one last commit.

aaltay · 2017-03-08T20:01:25Z

sdks/python/apache_beam/utils/value_provider_test.py

+
+    # provide values at graph-construction time
+    # (options not provided here become of the type RuntimeValueProvider)
+    options = UserOptions1(['1.2'])


I may have asked this before. If that is the case point me to the answer please.

Does it make sense to wrap things as value providers when template_location flag is not set. If the flag is not set we know that RVP values will be missing because it will not be run as a template.

No, you haven't.
Sure, I will add a warning for the case where there are RuntimeValueProvider arguments and template_location is missing.

Should it be an error or a warning? What is the behavior in Java SDK?

I am not finding anything in Java to enforce the template_location, so I am not adding this at this point, in order to be able to merge this PR. Should I open a JIRA issue for this to be done_ If so, I guess two JIRA issues, one for python and one for java?

Sounds good. Please reference the JIRA issue here.

coveralls · 2017-03-13T07:18:31Z

Coverage decreased (-0.003%) to 70.131% when pulling 6351e5a on mariapython:ppp_inmaster into 781e417 on apache:master.

asfbot · 2017-03-13T07:19:16Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8347/
--none--

coveralls · 2017-03-13T21:02:32Z

Changes Unknown when pulling 5a7e5ff on mariapython:ppp_inmaster into ** on apache:master**.

asfbot · 2017-03-13T21:03:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8361/
--none--

coveralls · 2017-03-14T19:07:57Z

Coverage increased (+0.01%) to 70.181% when pulling 761b38a on mariapython:ppp_inmaster into 843b663 on apache:master.

asfbot · 2017-03-14T19:09:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8394/
--none--

Incorporate a BeamArgumentParser (argparse.ArgumentParser + ValueProviders). Add StaticValueProvider and RuntimeValueProvider derived from ValueProvider. Add serialization for ValueProvider objects. Add testing for ValueProvider objects. Modify FileBasedSource and FileSink to accept ValueProvider objects.

coveralls · 2017-03-14T22:30:44Z

Coverage decreased (-0.003%) to 70.177% when pulling 0f9b1d1 on mariapython:ppp_inmaster into cc12fd3 on apache:master.

asfbot · 2017-03-14T22:31:26Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8409/
--none--

coveralls · 2017-03-14T22:37:52Z

Coverage decreased (-0.02%) to 70.164% when pulling 0f9b1d1 on mariapython:ppp_inmaster into cc12fd3 on apache:master.

asfbot · 2017-03-14T22:40:26Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8410/

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

org.apache.beam.runners.spark.translation.streaming.CreateStreamTest.testFirstElementLate

--none--

aaltay · 2017-03-15T16:54:15Z

retest this please

coveralls · 2017-03-15T17:35:32Z

Coverage increased (+0.002%) to 70.183% when pulling 0f9b1d1 on mariapython:ppp_inmaster into cc12fd3 on apache:master.

asfbot · 2017-03-15T17:37:07Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8452/
--none--

aaltay · 2017-03-15T19:10:13Z

LGTM. Thank you @mariapython

mariapython · 2017-03-21T19:57:38Z

Run Python PostCommit

mariapython force-pushed the ppp_inmaster branch from f0dab8f to aaa6963 Compare February 21, 2017 17:40

mariapython force-pushed the ppp_inmaster branch from aaa6963 to 9560065 Compare February 22, 2017 06:25

aaltay reviewed Feb 23, 2017

View reviewed changes

mariapython force-pushed the ppp_inmaster branch from 9560065 to 9aff2a5 Compare February 27, 2017 03:10

mariapython force-pushed the ppp_inmaster branch from 9aff2a5 to 97a3b53 Compare March 1, 2017 06:25

mariapython force-pushed the ppp_inmaster branch from 97a3b53 to 2916651 Compare March 1, 2017 21:57

mariapython force-pushed the ppp_inmaster branch from 2916651 to 9bdd605 Compare March 3, 2017 17:20

aaltay reviewed Mar 3, 2017

View reviewed changes

mariapython force-pushed the ppp_inmaster branch from 9bdd605 to 1ff44d5 Compare March 6, 2017 07:54

mariapython force-pushed the ppp_inmaster branch 2 times, most recently from a6b5196 to c5569ca Compare March 8, 2017 06:17

aaltay reviewed Mar 8, 2017

View reviewed changes

mariapython force-pushed the ppp_inmaster branch from c5569ca to 6351e5a Compare March 13, 2017 06:41

mariapython force-pushed the ppp_inmaster branch from 6351e5a to 5a7e5ff Compare March 13, 2017 20:24

mariapython force-pushed the ppp_inmaster branch from 5a7e5ff to 761b38a Compare March 14, 2017 18:33

mariapython force-pushed the ppp_inmaster branch from 761b38a to dd37fb6 Compare March 14, 2017 21:54

mariapython force-pushed the ppp_inmaster branch from dd37fb6 to 0f9b1d1 Compare March 14, 2017 21:56

asfgit closed this in 49c29f1 Mar 15, 2017

mariapython deleted the ppp_inmaster branch April 26, 2017 22:56

Conversation

mariapython commented Feb 8, 2017

Uh oh!

coveralls commented Feb 8, 2017

Uh oh!

asfbot commented Feb 8, 2017

Uh oh!

mariapython commented Feb 8, 2017

Uh oh!

coveralls commented Feb 21, 2017

Uh oh!

asfbot commented Feb 21, 2017

Uh oh!

coveralls commented Feb 22, 2017

Uh oh!

asfbot commented Feb 22, 2017

Uh oh!

aaltay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 27, 2017

Uh oh!

asfbot commented Feb 27, 2017

Uh oh!

asfbot commented Mar 1, 2017

Build result: FAILURE

Uh oh!

coveralls commented Mar 1, 2017

Uh oh!

asfbot commented Mar 1, 2017

Uh oh!

coveralls commented Mar 3, 2017

Uh oh!

asfbot commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariapython Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mariapython Mar 6, 2017 •

edited

Loading

mariapython Mar 6, 2017 •

edited

Loading