Update gen_protos to work with new protobuf on os-x. #25914

robertwb · 2023-03-21T15:54:07Z

Apparently the c-compiled protos are no longer available by default.

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

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
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.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

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

robertwb · 2023-03-21T15:54:18Z

R: @AnandInguva

github-actions · 2023-03-21T15:55:27Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

robertwb · 2023-03-21T21:06:20Z

The failures in apache_beam.runners.interactive.sql.utils_test.OptionsFormTest.test_dataflow_options_form look irrelevant.

robertwb · 2023-03-21T21:06:35Z

Run Python_Runners PreCommit

robertwb · 2023-03-21T21:06:45Z

Run Python_Coverage PreCommit

Apparently the c-compiled protos are no longer available by default.

tvalentyn · 2023-03-21T23:00:06Z

Run Python_Runners PreCommit

tvalentyn · 2023-03-21T23:00:18Z

Run Python_Coverage PreCommit

tvalentyn · 2023-03-22T01:12:27Z

sdks/python/gen_protos.py

+        _message.RepeatedCompositeContainer,
+    )  # pylint: disable=c-extension-no-member
+  except ImportError:
+    from google.protobuf.internal import containers


could you add a comment in which cases you expect else branch be used?

Yes, that would be helpful. I haven't caught this error while i was using when PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=upb(which is default).
but when PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION==python(slow implementation of protobufs which we don't want to use) I was using except case.

I've refactored this to be more explicit. Apparently the default implementation depends on the platform (and what wheels are shipped for it). Fast protos shouldn't matter here, and we are (as we should) verifying that we have a compiled implementation in our containers.

tvalentyn

asked someone to look into broken build.

robertwb

PTAL

robertwb · 2023-03-22T16:36:02Z

sdks/python/gen_protos.py

+        _message.RepeatedCompositeContainer,
+    )  # pylint: disable=c-extension-no-member
+  except ImportError:
+    from google.protobuf.internal import containers


I've refactored this to be more explicit. Apparently the default implementation depends on the platform (and what wheels are shipped for it). Fast protos shouldn't matter here, and we are (as we should) verifying that we have a compiled implementation in our containers.

tvalentyn · 2023-03-22T17:13:04Z

Backend: Wrote response {'code': 1, 'exc_type': 'AttributeError', 'exc_msg': "module 'google._upb.message' has no attribute 'RepeatedScalarFieldContainer'"} to /tmp/pep517_get_requires_for_build_sdist-xkfitq5.json

tvalentyn · 2023-03-22T17:14:53Z

Would it be a problem if different implementations were used at pipeline submission vs runtime? Is this a scenario we may be facing (for example when one submits a pipeline from MacOS which presumably doesn't have the right wheels)?

AnandInguva · 2023-03-22T17:21:42Z

sdks/python/gen_protos.py

+        "Unknown proto implementation: " + api_implementation.Type())
+  repeated_types = (
+      list,
+      impl.RepeatedScalarFieldContainer,


RepeatedScalarContainer should be used when it is google.protobuf.pyext._message or google._upb import _message

Oh, good point. Fixed.

robertwb · 2023-03-22T17:46:18Z

Regarding different implementations at compile vs. runtime, this is fine, as this code is just to do proto introspection to auto-generate the urn files to make type checkers more robust.

codecov · 2023-03-22T18:30:44Z

Codecov Report

Merging #25914 (67c19b1) into master (b5ce110) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 67c19b1 differs from pull request most recent head 8d1a5ec. Consider uploading reports for the commit 8d1a5ec to get more accurate results

@@            Coverage Diff             @@
##           master   #25914      +/-   ##
==========================================
- Coverage   71.41%   71.41%   -0.01%     
==========================================
  Files         778      778              
  Lines      102379   102435      +56     
==========================================
+ Hits        73112    73149      +37     
- Misses      27811    27830      +19     
  Partials     1456     1456

Flag	Coverage Δ
python	`79.95% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

AnandInguva

LGTM. seems like there is a lint error.

tvalentyn · 2023-03-23T17:52:53Z

yes, I understand this code is not relevant for job submission. My question is whether we should anticipate issues for some users of Mac OS b/c they will be using a different protobuf implementation.

robertwb · 2023-03-23T18:48:07Z

Ah. I hope not; the various implementations (or at least their public APIs) should be drop in replacements for each other. It's possible local runner performance may suffer, but probably not by that much, and it was never targeted at high efficiency anyway.

…

On Thu, Mar 23, 2023 at 10:53 AM tvalentyn ***@***.***> wrote: yes, I understand this code is not relevant for job submission. My question is whether we should anticipate issues for some users of Mac OS b/c they will be using a different protobuf implementation. — Reply to this email directly, view it on GitHub <#25914 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADWVAM7UBASMZNWPRC5MA3W5SEYBANCNFSM6AAAAAAWCUPNJA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

robertwb requested a review from tvalentyn March 21, 2023 15:54

github-actions bot added the python label Mar 21, 2023

Update gen_protos to work with new protobuf on os-x.

50f2e3d

Apparently the c-compiled protos are no longer available by default.

robertwb force-pushed the gen-protos branch from bbc8b09 to 50f2e3d Compare March 21, 2023 21:25

tvalentyn reviewed Mar 22, 2023

View reviewed changes

tvalentyn approved these changes Mar 22, 2023

View reviewed changes

AnandInguva mentioned this pull request Mar 22, 2023

Allow PTransforms to be applied directly to dataframes. #25919

Merged

3 tasks

robertwb added 2 commits March 22, 2023 09:32

More explicit proto impl type handling.

3ce3224

better name

99bd447

robertwb commented Mar 22, 2023

View reviewed changes

robertwb mentioned this pull request Mar 22, 2023

Add a source and sink option to yaml pipelines. #25935

Merged

3 tasks

AnandInguva reviewed Mar 22, 2023

View reviewed changes

accommodate different type names

67c19b1

AnandInguva approved these changes Mar 22, 2023

View reviewed changes

AnandInguva reviewed Mar 22, 2023

View reviewed changes

lint bug

8d1a5ec

robertwb merged commit fcf51fb into apache:master Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update gen_protos to work with new protobuf on os-x. #25914

Update gen_protos to work with new protobuf on os-x. #25914

robertwb commented Mar 21, 2023

robertwb commented Mar 21, 2023

github-actions bot commented Mar 21, 2023

robertwb commented Mar 21, 2023

robertwb commented Mar 21, 2023

robertwb commented Mar 21, 2023

tvalentyn commented Mar 21, 2023

tvalentyn commented Mar 21, 2023

tvalentyn Mar 22, 2023

AnandInguva Mar 22, 2023 •

edited

Loading

robertwb Mar 22, 2023

tvalentyn left a comment

robertwb left a comment

robertwb Mar 22, 2023

tvalentyn commented Mar 22, 2023

tvalentyn commented Mar 22, 2023

AnandInguva Mar 22, 2023 •

edited

Loading

robertwb Mar 22, 2023

robertwb commented Mar 22, 2023

codecov bot commented Mar 22, 2023 •

edited

Loading

AnandInguva left a comment

tvalentyn commented Mar 23, 2023

robertwb commented Mar 23, 2023 via email

Update gen_protos to work with new protobuf on os-x. #25914

Update gen_protos to work with new protobuf on os-x. #25914

Conversation

robertwb commented Mar 21, 2023

GitHub Actions Tests Status (on master branch)

robertwb commented Mar 21, 2023

github-actions bot commented Mar 21, 2023

robertwb commented Mar 21, 2023

robertwb commented Mar 21, 2023

robertwb commented Mar 21, 2023

tvalentyn commented Mar 21, 2023

tvalentyn commented Mar 21, 2023

tvalentyn Mar 22, 2023

Choose a reason for hiding this comment

AnandInguva Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

robertwb Mar 22, 2023

Choose a reason for hiding this comment

tvalentyn left a comment

Choose a reason for hiding this comment

robertwb left a comment

Choose a reason for hiding this comment

robertwb Mar 22, 2023

Choose a reason for hiding this comment

tvalentyn commented Mar 22, 2023

tvalentyn commented Mar 22, 2023

AnandInguva Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

robertwb Mar 22, 2023

Choose a reason for hiding this comment

robertwb commented Mar 22, 2023

codecov bot commented Mar 22, 2023 • edited Loading

Codecov Report

AnandInguva left a comment

Choose a reason for hiding this comment

tvalentyn commented Mar 23, 2023

robertwb commented Mar 23, 2023 via email

AnandInguva Mar 22, 2023 •

edited

Loading

AnandInguva Mar 22, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading