[BEAM-11408, BEAM-11772] Add explicit output typehints to ensure coder determinism for BQ with auto-sharding #14499

nehsyc · 2021-04-09T18:00:46Z

Also fix the string representation of ShardedKeyCoder to print out the key coder.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
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.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	ULR	Samza	Twister2
Go	---	---	---
Java
Python	---	---	---
XLang	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

…toBatches.WithShardedKey

nehsyc · 2021-04-09T18:01:01Z

R: @udim

udim · 2021-04-09T20:08:54Z

sdks/python/apache_beam/io/gcp/bigquery_file_loads.py

@@ -941,7 +943,8 @@ def _write_files_with_auto_sharding(
    destination_files_kv_pc = (
        destination_data_kv_pc
        | 'ToHashableTableRef' >> beam.Map(
-            lambda kv: (bigquery_tools.get_hashable_destination(kv[0]), kv[1]))
+            lambda kv: (bigquery_tools.get_hashable_destination(kv[0]), kv[1])).


Same as above, but with the additional step of making this lambda a function (you can't add type annotations to lambdas).

udim · 2021-04-09T20:08:58Z

sdks/python/apache_beam/io/gcp/bigquery.py

@@ -1431,7 +1433,8 @@ def _restore_table_ref(sharded_table_ref_elems_kv):
            bigquery_tools.AppendDestinationsFn(self.table_reference),
            *self.table_side_inputs)
        | 'AddInsertIds' >> beam.ParDo(_StreamToBigQuery.InsertIdPrefixFn())
-        | 'ToHashableTableRef' >> beam.Map(_to_hashable_table_ref))
+        | 'ToHashableTableRef' >> beam.Map(_to_hashable_table_ref)
+    ).with_output_types(Tuple[str, Any])


The explicit Any discards type information. Could you try annotating the types in _to_hashable_table_ref instead?

V = TypeVar('V') def _to_hashable_table_ref(table_ref_elem_kv: Tuple[Union[str, TABLE_REF_TYPE], V]) -> Tuple[str, V]:

Thanks for the suggestion! Made the change accordingly.

codecov · 2021-04-09T23:06:30Z

Codecov Report

Merging #14499 (96ce928) into master (1b86266) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #14499      +/-   ##
==========================================
- Coverage   83.49%   83.47%   -0.02%     
==========================================
  Files         447      449       +2     
  Lines       58904    58978      +74     
==========================================
+ Hits        49183    49233      +50     
- Misses       9721     9745      +24

Impacted Files	Coverage Δ
...sdks/python/apache_beam/internal/metrics/metric.py
...e_beam/portability/api/beam_runner_api_pb2_urns.py
...s/python/apache_beam/testing/pipeline_verifiers.py
...rcs/sdks/python/apache_beam/typehints/typehints.py
...s/python/apache_beam/io/gcp/bigquery_file_loads.py
...38/build/srcs/sdks/python/apache_beam/io/avroio.py
...srcs/sdks/python/apache_beam/metrics/metricbase.py
...srcs/sdks/python/apache_beam/coders/avro_record.py
...nternal/clients/dataflow/dataflow_v1b3_messages.py
...uild/srcs/sdks/python/apache_beam/coders/coders.py
... and 886 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b86266...96ce928. Read the comment docs.

udim · 2021-04-10T01:52:03Z

sdks/python/apache_beam/io/gcp/bigquery.py

@@ -1417,7 +1417,12 @@ def _add_random_shard(element):
      value = element[1]
      return ((key, random.randint(0, DEFAULT_SHARDS_PER_DESTINATION)), value)

-    def _to_hashable_table_ref(table_ref_elem_kv):
+    V = TypeVar('V')
+    TABLE_REF_TYPE = TypeVar('TABLE_REF_TYPE')


Sorry I was lazy when I wrote TABLE_REF_TYPE - I meant it should be replaced with bigquery.TableReference. (the Union should contain the types accepted by bigquery_tools.get_hashable_destination)

Aha. That makes more sense! Corrected.

udim · 2021-04-10T01:53:08Z

sdks/python/apache_beam/io/gcp/bigquery.py

+    V = TypeVar('V')
+    TABLE_REF_TYPE = TypeVar('TABLE_REF_TYPE')
+
+    def _to_hashable_table_ref(


This could be put in bigquery_tools.py to avoid code repetition.

Good idea. I moved it to bigquery_tools.py

nehsyc · 2021-04-10T06:01:02Z

retest this please

Move to_hashable_table_ref to bigquery_tools

nehsyc · 2021-04-10T22:01:11Z

sdks/python/apache_beam/io/gcp/bigquery_tools.py

@@ -69,6 +71,11 @@
 except ImportError:
  pass

+try:


I had to explicitly import this otherwise many tests failed with "apache_beam.io.gcp.internal.clients.bigquery has no attribute TableReference" - I guess bigquery lib was not available in some environments. The pylint check also failed possibly because of this but I haven't figured out what's wrong. Any insights?

that makes sense - you can add # pylint: disable=ungrouped-imports, wrong-import-order, wrong-import-position before the block to get pylint to pass.

Add explicit output typehints to make coder deterministic for GroupIn…

b4807b2

…toBatches.WithShardedKey

udim reviewed Apr 9, 2021

View reviewed changes

udim reviewed Apr 10, 2021

View reviewed changes

nehsyc force-pushed the deterministic_coder_fix branch 2 times, most recently from f9956e5 to 62768fd Compare April 10, 2021 06:36

Replace Any with TypeVar

38f8390

Move to_hashable_table_ref to bigquery_tools

nehsyc force-pushed the deterministic_coder_fix branch from 62768fd to 38f8390 Compare April 10, 2021 07:00

nehsyc commented Apr 10, 2021

View reviewed changes

try fixing link error

96ce928

pabloem merged commit f805f1c into apache:master Apr 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BEAM-11408, BEAM-11772] Add explicit output typehints to ensure coder determinism for BQ with auto-sharding #14499

[BEAM-11408, BEAM-11772] Add explicit output typehints to ensure coder determinism for BQ with auto-sharding #14499

Uh oh!

nehsyc commented Apr 9, 2021

Uh oh!

nehsyc commented Apr 9, 2021

Uh oh!

udim Apr 9, 2021

Uh oh!

udim Apr 9, 2021

Uh oh!

nehsyc Apr 9, 2021

Uh oh!

codecov bot commented Apr 9, 2021 •

edited

Loading

Uh oh!

udim Apr 10, 2021

Uh oh!

nehsyc Apr 10, 2021

Uh oh!

udim Apr 10, 2021

Uh oh!

nehsyc Apr 10, 2021

Uh oh!

nehsyc commented Apr 10, 2021

Uh oh!

nehsyc Apr 10, 2021

Uh oh!

pabloem Apr 12, 2021

Uh oh!

Uh oh!

[BEAM-11408, BEAM-11772] Add explicit output typehints to ensure coder determinism for BQ with auto-sharding #14499

[BEAM-11408, BEAM-11772] Add explicit output typehints to ensure coder determinism for BQ with auto-sharding #14499

Uh oh!

Conversation

nehsyc commented Apr 9, 2021

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

nehsyc commented Apr 9, 2021

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!

codecov bot commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

nehsyc commented Apr 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Apr 9, 2021 •

edited

Loading