Pass pipeline options to all call sites.#37509
Pass pipeline options to all call sites.#37509claudevdm wants to merge 1 commit intoapache:masterfrom
Conversation
Summary of ChangesHello @claudevdm, 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 significantly refactors how Apache Beam's Python SDK handles version compatibility within its coding mechanisms. By introducing a dedicated method in 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. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/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 refactors how pipeline options, particularly for version compatibility, are handled throughout the coder and transform components. A new method PipelineOptions.is_compat_version_prior_to is introduced to centralize version checking, and the options object is now passed explicitly through various method calls (like as_deterministic_coder) instead of relying on global state. This is a significant improvement for maintainability and correctness. My review focuses on ensuring the options parameter is propagated correctly everywhere. I've found a few places where this seems to have been missed.
claudevdm
force-pushed
the
pipelineoptions-compat
branch
from
a17f3dd to
86726f9
Compare
claudevdm
force-pushed
the
pipelineoptions-compat
branch
from
86726f9 to
3f45657
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring to pass PipelineOptions to various coder-related functions, instead of relying on a global state in typecoders.registry. This greatly improves the design by making dependency on pipeline options explicit, which enhances maintainability and prevents potential issues in multi-pipeline scenarios.
Key changes include:
- Adding an
optionsparameter toas_deterministic_codermethods and other related functions. - Introducing a new
PipelineOptions.is_compat_version_prior_tomethod to centralize version compatibility checks, and removing the old utility functions. - Updating
PipelineContextto hold and propagatePipelineOptions. - Thoroughly updating call sites and tests to use the new pattern.
The changes are consistent and well-tested, with new tests added for the new logic. This is a high-quality contribution. I have one suggestion to improve the test code by formally adding a test-only option instead of using object.__setattr__.
| options = PipelineOptions(update_compatibility_version=compat_version) | ||
| object.__setattr__(options, 'force_dill_deterministic_coders', True) |
There was a problem hiding this comment.
Using object.__setattr__ to set a test-only option can be a bit opaque. To make this cleaner and more explicit, consider adding force_dill_deterministic_coders as a formal option to TestingOptions in sdks/python/apache_beam/options/pipeline_options.py.
You would add this to TestingOptions._add_argparse_args:
parser.add_argument(
'--force_dill_deterministic_coders',
default=False,
action='store_true',
help='(INTERNAL) For testing purposes. Force the use of dill-based '
'deterministic coders for backward compatibility testing.')This would allow you to set the option directly in the PipelineOptions constructor as suggested, making the test setup more readable and maintainable. This change should be applied to other tests using this pattern as well.
| options = PipelineOptions(update_compatibility_version=compat_version) | |
| object.__setattr__(options, 'force_dill_deterministic_coders', True) | |
| options = PipelineOptions( | |
| update_compatibility_version=compat_version, | |
| force_dill_deterministic_coders=True) |
1 participant
Please add a meaningful description for your change here
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.