provide repro script #1711
provide repro script #1711
Conversation
Summary of ChangesHello @chandra-siri, 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 introduces a new Python script designed to serve as a reproduction example for performing asynchronous bidirectional writes to Google Cloud Storage. The script leverages the underlying GAPIC client ( 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. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a script for demonstrating bidi writes. The script appears to be a work in progress, containing significant amounts of commented-out code, hardcoded values, and deviations from standard Python practices. My review focuses on cleaning up the code, improving its reusability and robustness, and aligning it with best practices. A critical concern is the use of a private module (_storage_v2), which is not suitable for a public-facing sample.
| import os | ||
| import time | ||
| import asyncio | ||
| from google.cloud import _storage_v2 as storage_v2 |
There was a problem hiding this comment.
The script imports _storage_v2, which is a private module as indicated by the leading underscore. Public samples should only use the public API of the library. Please use the public google.cloud.storage module if possible. If this feature is only available in the private module, this script might not be suitable as a public sample.
| async for response in response_stream: | ||
| # print("stream count:", count, "*" * 20) | ||
| # print(response) | ||
| # print("time elapsed", time_elapsed) | ||
| # print("stream count:", count, "*" * 20) | ||
| if response.resource is not None and (generation is None): | ||
|
|
||
| generation = response.resource.generation | ||
|
|
||
| # print("genration = ", generation) | ||
| if response.write_handle is not None and (write_handle is None): | ||
| write_handle = response.write_handle.handle | ||
| # print("write_handle = ", write_handle) | ||
|
|
||
| # if |
There was a problem hiding this comment.
This block contains a significant amount of commented-out print statements and a stray # if which seems to be leftover from development. This dead code should be removed to improve readability and maintainability.
async for response in response_stream:
if response.resource is not None and (generation is None):
generation = response.resource.generation
if response.write_handle is not None and (write_handle is None):
write_handle = response.write_handle.handle| # if not args.skip_open: | ||
| generation, write_handle = await open_file(filename, storage_async_client) | ||
| print("Opend file for writing, gen:", generation) | ||
| # if not args.skip_append: | ||
| # if generation is None: | ||
| # print("generation is none requesting it") | ||
| # object_metadata = await get_object( | ||
| # bucket_fullname=BUCKET_FULLNAME, object_name=filename | ||
| # ) | ||
| # generation = object_metadata.generation | ||
| # print("generation is ", generation) | ||
| # await append_to_file(filename, generation, storage_async_client, data, write_handle) |
There was a problem hiding this comment.
This section contains a lot of commented-out code which seems to be the main logic for appending to the file. The script as it is only opens the file but doesn't perform the append operation. The commented-out if conditions for skip_open and skip_append also suggest that the script is intended to be more configurable than it currently is. This commented code should be cleaned up and the intended logic should be implemented.
| """ | ||
| async def open_file('file_name') | ||
| async def append_to_file('file_name') | ||
| async def close('') | ||
| """ |
There was a problem hiding this comment.
The current block at the top of the file appears to be scratchpad notes rather than a proper module docstring. To improve clarity and adhere to Python conventions, it should be replaced with a concise docstring that explains the script's purpose.
"""A script to demonstrate bidi writes using the google-cloud-storage GAPIC client."""|
|
||
| # from async_get_object_metadata import get_object | ||
|
|
||
| BUCKET_NAME = "chandrasiri-rs" |
There was a problem hiding this comment.
The bucket name is hardcoded, which limits the script's reusability. Consider making it a command-line argument, similar to filename.
You can add the following to your ArgumentParser setup:
parser.add_argument("--bucket_name", required=True)And then use args.bucket_name here.
| def request_generator(): | ||
| for request in [first_request]: | ||
| yield request |
There was a problem hiding this comment.
The request_generator function can be simplified. The for loop over a single-element list is redundant. A simple yield is sufficient.
def request_generator():
yield first_request| requests=request_generator(), | ||
| metadata=( | ||
| ("x-goog-request-params", req_param), | ||
| ("x-goog-api-client", "gcloud-python-local/3.8.0"), |
There was a problem hiding this comment.
The x-goog-api-client header is hardcoded. This is likely for specific debugging purposes. For a general-purpose script, it's better to remove this line and let the client library set its default user-agent header.
| count = 0 | ||
| prev_time = time.monotonic_ns() | ||
| total_time = 0 | ||
| async for response in append_stream: | ||
| end_time = time.monotonic_ns() | ||
| elapsed_time = end_time - prev_time | ||
| total_time += elapsed_time | ||
| print(f"Response count: {count}:", response) | ||
| count += 1 |
There was a problem hiding this comment.
This block appears to be for debugging and performance measurement, printing each response and timing information. For a clean, reusable script, this should be removed. If you just need to consume the stream, an empty async for loop is sufficient.
async for _ in append_stream:
pass| count += 1 | ||
|
|
||
|
|
||
| async def main(): |
There was a problem hiding this comment.
To make this function more self-contained and easier to test, it should receive the parsed command-line arguments as a parameter instead of relying on a global args variable.
async def main(args):| parser = argparse.ArgumentParser() | ||
| parser.add_argument("--filename", required=True) | ||
| parser.add_argument("--skip_open", action="store_true") | ||
| parser.add_argument("--skip_append", action="store_true") | ||
| args = parser.parse_args() | ||
| # print(args.skip_open, args.skip_append) | ||
| # print("yo") | ||
| asyncio.run(main()) |
There was a problem hiding this comment.
It's a Python best practice to place script execution logic within an if __name__ == "__main__": block. This prevents the code from being executed when the module is imported elsewhere. This also provides a good place to call main with the parsed args.
if __name__ == "__main__":
parser = argparse.ArgumentParser()
parser.add_argument("--filename", required=True)
parser.add_argument("--skip_open", action="store_true")
parser.add_argument("--skip_append", action="store_true")
args = parser.parse_args()
asyncio.run(main(args))
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕