BEAM-13939: Restructure Protos to fix namespace conflicts #16961

thempatel · 2022-02-26T17:29:21Z

Generated protobuf files contain additional information about the messages and services they were compiled from such as the file path to the original source proto file. The protobuf runtime for Golang maintains a global registry of all protobufs being used by registering the descriptors using the file path to the source proto file. If multiple descriptors with the same source file path are registered to the global registry, then the initialization code will prevent startup by panic-ing and printing a message like this to stdout:

panic: proto: file "metrics.proto" is already registered
	previously from: "github.com/apache/beam/sdks/v2/go/pkg/beam/model/pipeline_v1"
	currently from:  "..."
See https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

This behavior in the Go Protocol Buffer SDK is unlikely to go away:

reflect/protoregistry: restore conflicting file names check

There are inconsistencies between implementations on what happens when a single program contains generated code for multiple files with the same source path. At least one canonical implementation (C++) will fail at link time. Others print warnings. Some silently resolve the registry conflict in favor of one file or the other. The protobuf maintainers agree, however, that the desired behavior is for this condition to be an error.

This change aims to bring the protobuf imports in Beam to follow the guidance from a comment in this issue filed with the protobuf repo:

To avoid conflicts, every source .proto that is part of a program must have a unique name. Practically speaking, this means that any .proto that is part of a public package should be placed in a directory that uniquely identifies the owner of the proto. For example, google/protobuf/descriptor.proto and k8s.io/apimachinery/pkg/types both have prefixes which specify the creating organization and a specific package within that organization.

As such, I've elected to place each protobuf package in a directory org/apache/beam/model relative to its respective module root and have updated the build system where necessary.

P.S. This is still a work in progress, but opened the PR to socialize since this will affect all of Beam.

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.

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.

codecov · 2022-02-28T03:40:54Z

Codecov Report

Merging #16961 (bbf3a61) into master (e596abf) will decrease coverage by 0.02%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##           master   #16961      +/-   ##
==========================================
- Coverage   74.12%   74.09%   -0.03%     
==========================================
  Files         677      681       +4     
  Lines       89069    89209     +140     
==========================================
+ Hits        66019    66098      +79     
- Misses      21899    21960      +61     
  Partials     1151     1151

Flag	Coverage Δ
go	`50.08% <ø> (ø)`
python	`83.61% <98.43%> (-0.06%)`	⬇️

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

Impacted Files	Coverage Δ
...eam/runners/portability/fn_api_runner/execution.py	`92.25% <ø> (ø)`
...nners/portability/fn_api_runner/worker_handlers.py	`79.34% <0.00%> (ø)`
sdks/python/apache_beam/io/gcp/bigquery.py	`63.63% <100.00%> (ø)`
sdks/python/apache_beam/portability/common_urns.py	`100.00% <100.00%> (ø)`
...eam/runners/interactive/caching/streaming_cache.py	`96.66% <100.00%> (+0.01%)`	⬆️
...am/runners/interactive/options/capture_limiters.py	`90.76% <100.00%> (-0.14%)`	⬇️
...ache_beam/runners/interactive/recording_manager.py	`96.58% <100.00%> (ø)`
.../runners/interactive/testing/test_cache_manager.py	`92.75% <100.00%> (-0.11%)`	⬇️
...ks/python/apache_beam/runners/interactive/utils.py	`95.41% <100.00%> (ø)`
sdks/python/apache_beam/testing/test_stream.py	`91.02% <100.00%> (-0.06%)`	⬇️
... and 72 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 e596abf...bbf3a61. Read the comment docs.

thempatel · 2022-02-28T15:21:04Z

For python, thinking it might actually be better to make the structure of apache_beam.portability.api opaque, and find some way to hoist the modules within so we don't have to change imports in the rest of the SDK, hoping to find time to investigate

lukecwik

Should the proto package name be used as the basis for the directory name?
(e.g. org.apache.beam.model.job_management.v1 -> org.apache/beam/model/job_management/v1 and org.apache.beam.model.fn_execution.v1 -> org/apache/beam/model/fn_execution/v1)

lukecwik · 2022-02-28T23:30:56Z

R: @lukecwik

sdks/python/apache_beam/portability/api/__init__.py

thempatel · 2022-03-09T05:08:27Z

Is there an easy way to run these CI tasks locally? I'm trying some gradle tasks and some work, some don't and some that used to work don't work 😬 . Want to speed up my iteration speed, if there's a doc/readme I can read somewhere, that would be great too!

thempatel · 2022-03-09T05:11:27Z

I will start here

thempatel · 2022-03-17T23:52:19Z

sdks/python/container/Dockerfile

@@ -97,7 +97,7 @@ RUN rm /opt/apache/beam/third_party_licenses/golang/LICENSE

 COPY target/license_scripts /tmp/license_scripts/
 RUN if [ "$pull_licenses" = "true" ] ; then \
-      pip install 'pip-licenses<3.0.0' pyyaml tenacity && \
+      pip install 'pip-licenses>=3.5.3' pyyaml tenacity && \


raimon49/pip-licenses#113

thempatel · 2022-03-18T01:31:39Z

retest this please

thempatel · 2022-03-18T16:00:00Z

retest this please

thempatel · 2022-03-18T16:54:26Z

@lostluck @lukecwik I think (🤞🏽 ) this is ready for a formal review

thempatel · 2022-03-18T18:04:23Z

retest this please

lukecwik · 2022-03-18T19:35:30Z

Run Java PreCommit

lukecwik · 2022-03-18T19:35:39Z

Run GoPortable PreCommit

lukecwik · 2022-03-18T19:40:03Z

Looks good for me for the proto file changes.
R: @tvalentyn Can you review the python changes?
R: @lostluck Can you review the go changes?

lostluck · 2022-04-01T01:49:11Z

So the vast majority of Python changes seem to be unrelated to this (mostly go specific, already very large) change. Is there a way we could break them out and review them separately? The one change I see is that we (now) have to do renaming of the proto files in apache_beam/portability/api back to a flat structure (and fix their internal imports).

Milan can confirm, but since the changes are rooted in changing how the proto files import each other, I don't know how separable each change at a PR level.

Might be able to organize the changes as commits with: "proto file changes" "generator changes" "go generated code" "python things."... granularity though.

I'm ambivalent, since the go side is finished review at this point, and I don't understand the nature of the python side of the change.

thempatel · 2022-04-01T11:55:11Z

So the vast majority of Python changes seem to be unrelated to this (mostly go specific, already very large) change. Is there a way we could break them out and review them separately?

I sympathize with this, it's usually good advice to break large changes into their logical components and land them separately. I think in this case, that will be a challenge; I see the change-set here as one logical component. The nature of building a polyglot SDK on top of proto is that if you change the proto, you have to change all the languages and there's really no way around this without some long migration path. To that end, I think the per-language changes here are isolated enough where you can likely hide the changes from other files and review just the python files, a similar experience to what it would be like if we housed them in their own PR. Maybe a question to ask is what we do if something inexplicably goes wrong with merging this PR: is it easier to recover quickly if we have this change set spread over multiple commits, or a single commit? A revert of 1 commit is easy, a revert of multiple commits with others interspersed is much harder. If you feel really strongly that the python changes should be in their own change set, I am happy to oblige.

The one change I see is that we (now) have to do renaming of the proto files in apache_beam/portability/api back to a flat structure (and fix their internal imports).

Let's chat about this, I'm not sure I understand what is being said here. Why would we have to change this back to a flat structure? I think that will be impossible, hence the extensive changes in the generation tooling.

Also, for auto-generated files, I think adding them to RAT is better than modifying these files--they're (clearly marked) machine output.

It looks like the generated go files are already in the RAT, though I do agree with Robert that maintaining the ASF license header is a better avenue, plus it reduces the diff on the PR

robertwb · 2022-04-01T17:50:01Z

Just to confirm, imports of the form from apache_beam.portability.api.beam_runner_api_pb2 import TestStreamPayload are no longer supported, and that's why the diff is so huge?

thempatel · 2022-04-01T18:35:33Z

Just to confirm, imports of the form from apache_beam.portability.api.beam_runner_api_pb2 import TestStreamPayload are no longer supported, and that's why the diff is so huge?

correct, this PR changes the the proto structure from a flat one to a hierarchical one, like so:

model/
  beam_fn_api.proto
  beam_provision_api.proto
  beam_interactive_api.proto
  beam_artifact_api.proto
  ...

apache_beam/portability/api/
  beam_fn_api_pb2.py
  beam_provision_api_pb2.py
  beam_interactive_api_pb2.py
  beam_artifact_api_pb2.py
  ...

to

model/org/apache/beam/model/
    beam_fn_api.proto
    beam_provision_api.proto
    beam_interactive_api.proto
    beam_artifact_api.proto
    ...

apache_beam/portability/api/org/apache/beam/model/
    beam_fn_api_pb2.py
    beam_provision_api_pb2.py
    beam_interactive_api_pb2.py
    beam_artifact_api_pb2.py
    ...

so the reason you can no longer do from apache_beam.portability.api.beam_runner_api_pb2 import TestStreamPayload is because there is no module there! The fully qualified import is now

from apache_beam.portability.api.org.apache.beam.model.pipeline.beam_runner_api_pb2 import TestStreamPayload

In order to make this easier to work with, I updated the proto generator to also generate module bindings in the __init__ file of apache_beam.portability.api, like so

from .org.apache.beam.model import pipeline
# ...

external_transforms_pb2 = pipeline.external_transforms_pb2
# ...

so that you don't need to provide the fully qualified path. If we didn't do this, this PR would be even more huge, since we'd have to update every import of the generated bindings in the SDK

sdks/go/pkg/beam/transforms/stats/util_gen.tmpl

sdks/python/.yapfignore

sdks/python/apache_beam/portability/api/__init__.py

robertwb · 2022-04-01T22:34:46Z

sdks/python/gen_protos.py

+LOG = logging.getLogger()
+LOG.setLevel(logging.INFO)
+
+LICENSE_HEADER = """


I would just drop these two headers and skip the checks over adding logic to prepend them to the auto-generated files.

If you're not strongly opposed, I suggest we keep these. My argument is less related to the build system, and more related to messaging to consumers of the SDK: if they are exploring the SDK, it is a useful notice to have so that they know this is Beam's posture

sdks/python/gen_protos.py

lostluck

LGTM.

Unless there are any further objections, I can merge this in later this week.

lostluck · 2022-04-05T23:25:08Z

I guess @lukecwik needs to validate/approve his requested changes first.

…structure. This is so that the proto files require usage of a org/apache/beam/model namespace in their imports and so that the generated files also include this namespace in their source file metadata.

…enerate all proto files for the go sdk. This new tool will add any necessary options to the proto compiler and generate the proto files relative to the go sdk root to ensure that the generated files have a namespaced file path in their metadata. If you want to generate a proto file in the go sdk, simply use this script in the go:generate directive, the rest will be taken care of by the script.

…te proto bindings. Updates the README for how to generate the model proto bindings into the SDK

…w namespaced structure of the Beam model. It does this by supporting arbitrary directory structures of proto files by calculating and replacing the generated imports with relative imports with the generated source. Additionally, it will generate bindings that allow for imports of the form `from apache_beam.portability.api import beam_runner_api_pb2` so that the SDK is not dependent on the potentially changing structure of the generated bindings within `api`. Imports of the form `from apache_beam.portability.api.org.apache.beam.model import beam_runner_api_pb2` are still supported. setup.py now attempts to generate the proto bindings on invocation since the package structure must exist before the wheel can be created.

…rder to support the new python output structure

…gen_protos

thempatel · 2022-04-08T12:45:32Z

thanks @lostluck @lukecwik @tvalentyn @robertwb for the thoughtful reviews, i had fun on this one! 🙏🏽

asf-ci · 2022-04-08T16:07:57Z

Can one of the admins verify this patch?

asf-ci · 2022-04-08T16:07:58Z

Can one of the admins verify this patch?

github-actions bot added build go model python labels Feb 26, 2022

thempatel force-pushed the milan/fix-proto-import-paths branch from 4d96ebf to 3ee7195 Compare February 27, 2022 16:54

thempatel force-pushed the milan/fix-proto-import-paths branch from 8e08f62 to c75cee8 Compare February 28, 2022 03:49

lostluck self-requested a review February 28, 2022 03:59

thempatel force-pushed the milan/fix-proto-import-paths branch from c75cee8 to 0a60f2a Compare February 28, 2022 03:59

github-actions bot added gcp io labels Feb 28, 2022

lukecwik requested changes Feb 28, 2022

View reviewed changes

github-actions bot removed io gcp labels Mar 7, 2022

thempatel commented Mar 8, 2022

View reviewed changes

sdks/python/apache_beam/portability/api/__init__.py Show resolved Hide resolved

github-actions bot added gcp io docker labels Mar 13, 2022

thempatel commented Mar 17, 2022

View reviewed changes

thempatel force-pushed the milan/fix-proto-import-paths branch 6 times, most recently from 2954739 to 81100c1 Compare April 1, 2022 13:55

robertwb approved these changes Apr 1, 2022

View reviewed changes

thempatel force-pushed the milan/fix-proto-import-paths branch 3 times, most recently from 23ce537 to 35f99b3 Compare April 2, 2022 21:45

lostluck approved these changes Apr 5, 2022

View reviewed changes

thempatel added 8 commits April 6, 2022 22:03

Updates the Beam model protos to be housed in a namespaced directory …

c54e923

…structure. This is so that the proto files require usage of a org/apache/beam/model namespace in their imports and so that the generated files also include this namespace in their source file metadata.

Updates session.proto to use the new namespaced import

ce48b4a

Updates the go sdk to use the new genproto script to correctly genera…

0c0db24

…te proto bindings. Updates the README for how to generate the model proto bindings into the SDK

Regenerates all go sdk proto bindings

53ba83d

Updates to various build, ignore, and linting tooling for python in o…

cb776c7

…rder to support the new python output structure

Updates to the python SDK to use the new import structure created by …

bbf3a61

…gen_protos

thempatel force-pushed the milan/fix-proto-import-paths branch from 35f99b3 to bbf3a61 Compare April 7, 2022 02:12

lukecwik merged commit e44d8a7 into apache:master Apr 7, 2022

thempatel deleted the milan/fix-proto-import-paths branch April 8, 2022 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BEAM-13939: Restructure Protos to fix namespace conflicts #16961

BEAM-13939: Restructure Protos to fix namespace conflicts #16961

thempatel commented Feb 26, 2022 •

edited

Loading

codecov bot commented Feb 28, 2022 •

edited

Loading

thempatel commented Feb 28, 2022

lukecwik left a comment

lukecwik commented Feb 28, 2022

thempatel commented Mar 9, 2022

thempatel commented Mar 9, 2022

thempatel Mar 17, 2022

thempatel commented Mar 18, 2022

thempatel commented Mar 18, 2022

thempatel commented Mar 18, 2022

thempatel commented Mar 18, 2022

lukecwik commented Mar 18, 2022

lukecwik commented Mar 18, 2022

lukecwik commented Mar 18, 2022

lostluck commented Apr 1, 2022

thempatel commented Apr 1, 2022

robertwb commented Apr 1, 2022

thempatel commented Apr 1, 2022 •

edited

Loading

robertwb Apr 1, 2022

thempatel Apr 2, 2022

lostluck left a comment

lostluck commented Apr 5, 2022

thempatel commented Apr 8, 2022

asf-ci commented Apr 8, 2022

asf-ci commented Apr 8, 2022

BEAM-13939: Restructure Protos to fix namespace conflicts #16961

BEAM-13939: Restructure Protos to fix namespace conflicts #16961

Conversation

thempatel commented Feb 26, 2022 • edited Loading

GitHub Actions Tests Status (on master branch)

codecov bot commented Feb 28, 2022 • edited Loading

Codecov Report

thempatel commented Feb 28, 2022

lukecwik left a comment

Choose a reason for hiding this comment

lukecwik commented Feb 28, 2022

thempatel commented Mar 9, 2022

thempatel commented Mar 9, 2022

thempatel Mar 17, 2022

Choose a reason for hiding this comment

thempatel commented Mar 18, 2022

thempatel commented Mar 18, 2022

thempatel commented Mar 18, 2022

thempatel commented Mar 18, 2022

lukecwik commented Mar 18, 2022

lukecwik commented Mar 18, 2022

lukecwik commented Mar 18, 2022

lostluck commented Apr 1, 2022

thempatel commented Apr 1, 2022

robertwb commented Apr 1, 2022

thempatel commented Apr 1, 2022 • edited Loading

robertwb Apr 1, 2022

Choose a reason for hiding this comment

thempatel Apr 2, 2022

Choose a reason for hiding this comment

lostluck left a comment

Choose a reason for hiding this comment

lostluck commented Apr 5, 2022

thempatel commented Apr 8, 2022

asf-ci commented Apr 8, 2022

asf-ci commented Apr 8, 2022

thempatel commented Feb 26, 2022 •

edited

Loading

codecov bot commented Feb 28, 2022 •

edited

Loading

thempatel commented Apr 1, 2022 •

edited

Loading