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 2022/04/05 00:36:41 UTC

[GitHub] [beam] robertwb opened a new pull request, #17278: [BEAM-14250] Fix coder registration for types defined in __main__.

robertwb opened a new pull request, #17278:
URL: https://github.com/apache/beam/pull/17278

   Until all runners are portable and we can get rid of all round trips
   between Pipeline and proto representatons, register types in __main__
   according to their string representations as pickling does not
   preserve identity.
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] robertwb commented on pull request #17278: [BEAM-14250] Fix coder registration for types defined in __main__.

Posted by GitBox <gi...@apache.org>.
robertwb commented on PR #17278:
URL: https://github.com/apache/beam/pull/17278#issuecomment-1088153598

   R: @yeandy 


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] commented on pull request #17278: [BEAM-14250] Fix coder registration for types defined in __main__.

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #17278:
URL: https://github.com/apache/beam/pull/17278#issuecomment-1089154389

   # [Codecov](https://codecov.io/gh/apache/beam/pull/17278?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#17278](https://codecov.io/gh/apache/beam/pull/17278?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0843149) into [master](https://codecov.io/gh/apache/beam/commit/0262ee53c6018d929a8a40fdf66735cc7e934951?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0262ee5) will **increase** coverage by `0.00%`.
   > The diff coverage is `50.00%`.
   
   ```diff
   @@           Coverage Diff           @@
   ##           master   #17278   +/-   ##
   =======================================
     Coverage   74.15%   74.15%           
   =======================================
     Files         676      676           
     Lines       89008    89012    +4     
   =======================================
   + Hits        66002    66006    +4     
     Misses      21859    21859           
     Partials     1147     1147           
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | python | `83.69% <50.00%> (+<0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/17278?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [sdks/python/apache\_beam/coders/typecoders.py](https://codecov.io/gh/apache/beam/pull/17278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vY29kZXJzL3R5cGVjb2RlcnMucHk=) | `92.40% <50.00%> (-2.27%)` | :arrow_down: |
   | [...che\_beam/runners/interactive/interactive\_runner.py](https://codecov.io/gh/apache/beam/pull/17278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9pbnRlcmFjdGl2ZS9pbnRlcmFjdGl2ZV9ydW5uZXIucHk=) | `92.25% <0.00%> (-0.71%)` | :arrow_down: |
   | [sdks/python/apache\_beam/runners/direct/executor.py](https://codecov.io/gh/apache/beam/pull/17278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kaXJlY3QvZXhlY3V0b3IucHk=) | `96.46% <0.00%> (-0.55%)` | :arrow_down: |
   | [...eam/runners/interactive/interactive\_environment.py](https://codecov.io/gh/apache/beam/pull/17278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9pbnRlcmFjdGl2ZS9pbnRlcmFjdGl2ZV9lbnZpcm9ubWVudC5weQ==) | `90.21% <0.00%> (-0.31%)` | :arrow_down: |
   | [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/17278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.51% <0.00%> (-0.13%)` | :arrow_down: |
   | [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/17278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `91.00% <0.00%> (+1.00%)` | :arrow_up: |
   | [...ks/python/apache\_beam/runners/worker/data\_plane.py](https://codecov.io/gh/apache/beam/pull/17278/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvZGF0YV9wbGFuZS5weQ==) | `89.20% <0.00%> (+1.70%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/17278?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/17278?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0262ee5...0843149](https://codecov.io/gh/apache/beam/pull/17278?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] robertwb merged pull request #17278: [BEAM-14250] Fix coder registration for types defined in __main__.

Posted by GitBox <gi...@apache.org>.
robertwb merged PR #17278:
URL: https://github.com/apache/beam/pull/17278


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] KevinGG commented on pull request #17278: [BEAM-14250] Fix coder registration for types defined in __main__.

Posted by GitBox <gi...@apache.org>.
KevinGG commented on PR #17278:
URL: https://github.com/apache/beam/pull/17278#issuecomment-1115475473

   This seems to have broken the `RowCoder.from_type_hint`.
   
   So for `SqlTransform` with `NamedTuple` defined in the `main` module, the below code will fail because `schema_from_element_type` does not know how to handle a `str` type hint:
   
   ```
   beam.coders.registry.register_coder(NamedTupleInMain, beam.coders.RowCoder)
   coder = beam.coders.registry.get_coder(NamedTupleInMain)
   ```
   
   I'll see if I can send an amendment to it.
   


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] yeandy commented on a diff in pull request #17278: [BEAM-14250] Fix coder registration for types defined in __main__.

Posted by GitBox <gi...@apache.org>.
yeandy commented on code in PR #17278:
URL: https://github.com/apache/beam/pull/17278#discussion_r842898174


##########
sdks/python/apache_beam/coders/typecoders.py:
##########
@@ -116,10 +116,18 @@ def register_coder(self, typehint_type, typehint_coder_class):
           'Received %r instead.' % typehint_coder_class)
     if typehint_type not in self.custom_types:
       self.custom_types.append(typehint_type)
+    if typehint_type.__module__ == '__main__':

Review Comment:
   Seems to be failures if `typehint_type` is None
   ```suggestion
       if typehint and typehint_type.__module__ == '__main__':
   ```



##########
sdks/python/apache_beam/coders/typecoders.py:
##########
@@ -116,10 +116,18 @@ def register_coder(self, typehint_type, typehint_coder_class):
           'Received %r instead.' % typehint_coder_class)
     if typehint_type not in self.custom_types:
       self.custom_types.append(typehint_type)
+    if typehint_type.__module__ == '__main__':
+      # See https://issues.apache.org/jira/browse/BEAM-14250
+      # TODO(robertwb): Remove once all runners are portable.
+      typehint_type = str(typehint_type)
     self._register_coder_internal(typehint_type, typehint_coder_class)
 
   def get_coder(self, typehint):
     # type: (Any) -> coders.Coder
+    if typehint.__module__ == '__main__':

Review Comment:
   ```suggestion
       if typehint and typehint.__module__ == '__main__':
   ```



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] KevinGG commented on a diff in pull request #17278: [BEAM-14250] Fix coder registration for types defined in __main__.

Posted by GitBox <gi...@apache.org>.
KevinGG commented on code in PR #17278:
URL: https://github.com/apache/beam/pull/17278#discussion_r863283190


##########
sdks/python/apache_beam/coders/typecoders.py:
##########
@@ -116,10 +116,18 @@ def register_coder(self, typehint_type, typehint_coder_class):
           'Received %r instead.' % typehint_coder_class)
     if typehint_type not in self.custom_types:
       self.custom_types.append(typehint_type)
+    if typehint_type.__module__ == '__main__':
+      # See https://issues.apache.org/jira/browse/BEAM-14250
+      # TODO(robertwb): Remove once all runners are portable.
+      typehint_type = str(typehint_type)

Review Comment:
   Actually `str()` might be non-deterministic for a Union type such as `Optional[Union[str, int]]`. So this could potentially break when it's stringified differently when register_coder and get_coder. An example flaky test we encountered in the past: https://github.com/apache/beam/pull/17351.



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org