Avoid RequestResponseIO throttler access when throttler is absent#39112
Avoid RequestResponseIO throttler access when throttler is absent#39112fallintoplace wants to merge 6 commits into
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 addresses a runtime error in RequestResponseIO by ensuring that the throttler's successful_request method is only invoked when a throttler instance is present. This change improves the robustness of the component by safely handling scenarios where throttling is not required or configured. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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
|
![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 adds a safety check to ensure self._throttler is set before calling successful_request. The feedback suggests using an explicit is not None check instead of implicit boolean evaluation to comply with PEP 8 guidelines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| self._caller, request, self._timeout, self._metrics_collector) | ||
| self._metrics_collector.responses.inc(1) | ||
| self._throttler.throttler.successful_request(req_time * MSEC_TO_SEC) | ||
| if self._throttler: |
There was a problem hiding this comment.
According to PEP 8, comparisons to singletons like None should always be done with is or is not, never the equality operators or implicit boolean evaluation when checking if an optional argument/attribute is set. Using is not None is safer and more explicit.
| if self._throttler: | |
| if self._throttler is not None: |
References
- PEP 8: Use 'is not None' when testing whether a variable or argument that defaults to None was set to some other value. (link)
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Assigning reviewers: R: @damccorm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| raise Exception( | ||
| f'Expected batch of size 2, received batch of size {len(batch)}') | ||
| 'Expected batch size 1 or 2, received batch of size ' | ||
| f'{len(batch)}') |
There was a problem hiding this comment.
Why are we including this change? It seems like this changes the meaning of the test (incorrectly)
In general, it is better to keep PRs focused on a single task
There was a problem hiding this comment.
@damccorm Sorry, this might come from incorrect branching on my behalf. Let me push the fix in the next hour.
damccorm
left a comment
damccorm
left a comment
There was a problem hiding this comment.
I'm supportive of the main change, but its not clear to me why we're including these test changes
2 participants
Description
_CallDoFn.process(apache_beam/io/requestresponse.py), only callself._throttler.throttler.successful_request(...)when a throttler is configured.RequestResponseIOwas used without a throttler on successful responses.AttributeError: 'NoneType' object has no attribute 'throttler'.Testing
Not run (not requested).