Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes the handling of Row types in Apache Beam by leveraging the schema registry. By reusing registered types rather than dynamically generating new named tuples, the system improves efficiency and ensures better type consistency across pipeline operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
R: @claudevdm |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
Actually this is not yet effective on Dataflow runner as schema registry hasn't been added to pickle |
|
It becomes tricky. The main session is written to local file here: as part of preparing python_sdk_dependencies Then the schema is registered a few lines later, it's not easy to redo save main session after this line, as the main session as a staged file has a checksum written in environment |
|
nevertheless the current progress still fixes an inefficiency path such that generated Namedtuple is created every time named_tuple_from_schema is called, write to schema registry but never honors it. Updated: solved by introducing a new "CoderRegistry.register_row" method |
|
"PreCommit Python ML tests with ML deps installed" also failing on HEAD, not related to the change. Ready for review for now. |
* Introduce register_row to register with both coder and schema registry Save schema registry id->type mapping
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a register_row method to the type coder registry to streamline the registration of user types with Beam Rows and their schemas. It also updates session pickling to include the schema registry, ensuring schema IDs are preserved across sessions. Additionally, the logic for generating named tuples from schemas was updated to reuse existing types from the registry. Feedback was provided to rename variables in the load_registered_typings method to avoid shadowing the built-in id() function and to improve clarity regarding the mapping of IDs to typings.
| def load_registered_typings(self, by_id): | ||
| for id, typing in by_id.items(): | ||
| if id not in self.by_id: | ||
| self.by_id[id] = (typing, None) |
There was a problem hiding this comment.
The variable name id shadows the built-in Python function id(). Additionally, the argument name by_id is slightly confusing because it contains a mapping of IDs to typings, whereas the class attribute self.by_id stores tuples of (typing, schema). Renaming these to id_to_typing and schema_id improves clarity and avoids potential conflicts with built-in names.
| def load_registered_typings(self, by_id): | |
| for id, typing in by_id.items(): | |
| if id not in self.by_id: | |
| self.by_id[id] = (typing, None) | |
| def load_registered_typings(self, id_to_typing): | |
| for schema_id, typing in id_to_typing.items(): | |
| if schema_id not in self.by_id: | |
| self.by_id[schema_id] = (typing, None) |
claudevdm
left a comment
claudevdm
left a comment
There was a problem hiding this comment.
Very nice thanks!
Can we run a tap train on this before merging/importing?
| self._register_coder_internal( | ||
| self._normalize_typehint_type(typehint_type), typehint_coder_class) | ||
|
|
||
| def register_row(self, typehint_type: Any) -> None: |
There was a problem hiding this comment.
What do you think of adding a decorator like @beam.row for conveinence?
Something like
@beam.row
@dataclass
class Order:
id: int
amount: float
def row(cls):
cls._beam_row_pending = True
return cls
Then when registering coders or RowTypeConstraint.from_user_type check for this attribute?
|
|
||
| beam.coders.typecoders.registry.register_coder( | ||
| MyNamedTuple, beam.coders.RowCoder) | ||
| beam.coders.typecoders.registry.register_row(MyNamedTuple) |
There was a problem hiding this comment.
I am not sure the divegence between beam.coders.typecoders.registry.register_coder and beam.coders.typecoders.registry.register_row is a good thing. Most users probably want the benefit of this PR but rely on register_coder.
Can we either
- make register_coder act the same as register_row OR
- Deprecate register_coder(T, RowCoder) with a warning. DeprecationWarning: use register_row(T) instead when the coder class is RowCoder
2 participants
Please add a meaningful description for your change here
Instead of generate named tuple every time
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.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 or the workflows README to see a list of phrases to trigger workflows.