Conversation
![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 support for resumable upload methods in the GAPIC generator, updating schema wrappers, templates, and options to handle media upload protocols, while adding comprehensive system and unit tests. Feedback on the changes suggests avoiding potential breaking changes by generating media upload methods as standard RPCs when resumable_upload_prefix is not configured, rather than omitting them entirely. Additionally, it is recommended to deduplicate helper functions like resume_resumable_upload and make_resumable_upload across the newly added system test files by moving them to a shared utility module or conftest.py.
parthea
changed the title
| dependencies = [ | ||
| "google-api-core[grpc] >= 2.28.0, <3.0.0", | ||
| # TODO: For prototyping purposes only, revert once https://github.com/googleapis/google-cloud-python/pull/18352 is merged | ||
| "google-api-core[grpc]@git+https://github.com/googleapis/google-cloud-python.git@feat/resumable-transfer-api-core#subdirectory=packages/google-api-core", |
There was a problem hiding this comment.
For prototyping purposes only, revert once #18352 is merged and released to PyPI
| # and https://github.com/actions/checkout#checkout-head. | ||
| with: | ||
| fetch-depth: 2 | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
I will revert this change once #18352 is merged to main and released
daniel-sanche
left a comment
daniel-sanche
left a comment
There was a problem hiding this comment.
Is it possible to break some of this up into separate PRs? There's a lot here
| # - It may require correct/in-range values for request initialization. | ||
| # - It may require specifying regional endpoints when creating the service | ||
| # client as shown in: | ||
| # https://googleapis.dev/python/google-api-core/latest/client_options.html |
There was a problem hiding this comment.
Is it standard to include this text within the region tags of every sample? Is this because it's in showcase?
There was a problem hiding this comment.
Yes, it is standard. It exists on all generated samples. See details in go/snippet-gen-design
|
|
||
| # Initialize request argument(s) | ||
| request = showcase_v1beta1.EnumRequest( | ||
| ) |
There was a problem hiding this comment.
do we need to run ruff on these? The formatting seems unexpected
There was a problem hiding this comment.
We don't run ruff on golden files. It's a post processing step (post generation). There is a similar formatting issue in other goldens. I will file a bug to follow up on whether we want to run ruff on goldens but let's tackle that separately since it's an existing issue.
There was a problem hiding this comment.
Filed #18439 to add to our backlog to decide if we want to run ruff on golden files
|
|
||
| # Iterate over the upload to receive progress updates as each chunk is transmitted | ||
| stream = io.BytesIO(b"Example upload data") | ||
| for progress in upload_session.iter_upload(stream): |
There was a problem hiding this comment.
nit: it would be helpful to add a type annotation here
| retry: OptionalRetry = gapic_v1.method.DEFAULT, | ||
| timeout: Union[float, object] = gapic_v1.method.DEFAULT, | ||
| metadata: Sequence[Tuple[str, Union[str, bytes]]] = (), | ||
| ) -> resumable_upload.UploadMediaResponse: |
There was a problem hiding this comment.
This seems to have a different signature than tha samples (i.e. it doesn't accept config, doesn't return ResumableUploadSession)
| self.get_operation, | ||
| default_timeout=None, | ||
| client_info=client_info, | ||
| kind='rest', |
There was a problem hiding this comment.
This doesn't look right, this is the grpc_asyncio transport
|
I've addressed all of the review comments. I'll split this up into separate PRs to reduce the diff/make the review easier |
cddfde0 to
65b0eed
Compare
This reverts commit eaba74b.
fc9bdd2 to
7539d1d
Compare
This branch has not been deployed
2 participants
Towards b/457416314, b/556259599