feat: implement native asyncio support via Cross-Sync#1509
Draft
sinhasubham wants to merge 4 commits intomainfrom
Draft
feat: implement native asyncio support via Cross-Sync#1509sinhasubham wants to merge 4 commits intomainfrom
sinhasubham wants to merge 4 commits intomainfrom
Conversation
Contributor
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 integrates native asyncio capabilities into the Highlights
Changelog
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
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant architectural change with the new Cross-Sync mechanism for generating synchronous code from an asynchronous source. The implementation of the code generation and the new async components is well-structured. However, I've identified a critical issue in an async transport interceptor where a missing await will likely cause runtime errors. Additionally, there are opportunities for improvement by refactoring duplicated code in the AST transformers, enhancing the robustness of AST checks, and correcting some documentation typos and inaccuracies. Addressing these points will further strengthen this excellent contribution towards native asyncio support.
| }, | ||
| ) | ||
| response = await continuation(client_call_details, request) | ||
| response = continuation(client_call_details, request) |
Contributor
There was a problem hiding this comment.
The await keyword was removed here, but continuation returns an awaitable. Without await, response will be a coroutine object, and subsequent calls like response.trailing_metadata() will fail. This should be response = await continuation(client_call_details, request).
Suggested change
| response = continuation(client_call_details, request) | |
| response = await continuation(client_call_details, request) |
|
|
||
| self._interceptor = _LoggingClientAIOInterceptor() | ||
| self._grpc_channel._unary_unary_interceptors.append(self._interceptor) | ||
| # self._grpc_channel._unary_unary_interceptors.append(self._interceptor) |
Contributor
There was a problem hiding this comment.
The logging interceptor is being commented out. Is this intentional? If so, it should be documented why. If it's because of the issue with await continuation(...), the fix should be to add the await back, not to disable the interceptor.
| - `CrossSync.gather_partials()` | ||
| - `CrossSync.wait()` | ||
| - `CrossSync.condition_wait()` | ||
| - `CrossSync,event_wait()` |
Contributor
There was a problem hiding this comment.
There's a typo here. It should be CrossSync.event_wait() instead of CrossSync,event_wait().
Suggested change
| - `CrossSync,event_wait()` | |
| - `CrossSync.event_wait()` |
| ### Code Generation | ||
|
|
||
| Generation can be initiated using `nox -s generate_sync` | ||
| from the root of the project. This will find all classes with the `__CROSS_SYNC_OUTPUT__ = "path/to/output"` |
Contributor
There was a problem hiding this comment.
There's a trailing space in the code example which could be confusing.
Suggested change
| from the root of the project. This will find all classes with the `__CROSS_SYNC_OUTPUT__ = "path/to/output"` | |
| from the root of the project. This will find all classes with the `__CROSS_SYNC_OUTPUT__ = "path/to/output"` |
| ## Architecture | ||
|
|
||
| CrossSync is made up of two parts: | ||
| - the runtime shims and annotations live in `/google/cloud/bigtable/_cross_sync` |
Contributor
There was a problem hiding this comment.
This path seems incorrect. Based on the file structure in this PR, it should probably be /google/cloud/aio/_cross_sync.
Suggested change
| - the runtime shims and annotations live in `/google/cloud/bigtable/_cross_sync` | |
| - the runtime shims and annotations live in `/google/cloud/aio/_cross_sync` |
| file_transformer = CrossSyncFileProcessor() | ||
| # run each file through ast transformation to find all annotated classes | ||
| for file_path in files: | ||
| ast_tree = ast.parse(open(file_path, encoding="utf-8-sig").read()) |
Contributor
There was a problem hiding this comment.
It's safer to use a with statement when opening files. This ensures the file is closed even if an error occurs during parsing. For example:
with open(file_path, encoding="utf-8-sig") as f:
ast_tree = ast.parse(f.read())
Comment on lines
+66
to
+76
| def visit_ImportFrom(self, node): | ||
| if node.module: | ||
| if "_async" in node.module: node.module = node.module.replace("._async", "").replace("_async.", "").replace("_async", "") | ||
| if "async_client" in node.module: node.module = node.module.replace("async_client", "client") | ||
| # Also replace AsyncClient with Client in the names! | ||
| for alias in node.names: | ||
| if "AsyncClient" in alias.name: | ||
| alias.name = alias.name.replace("AsyncClient", "Client") | ||
| if alias.name == "AsyncRetry": | ||
| alias.name = "Retry" | ||
| return self.generic_visit(node) |
Contributor
There was a problem hiding this comment.
The visit_ImportFrom method is duplicated in SymbolReplacer, AsyncToSync (line 146), and CrossSyncFileProcessor (line 363). This logic should be refactored into a single place, for example, a new NodeTransformer class, to avoid inconsistencies and improve maintainability.
| if not isinstance(root_attr, ast.Attribute): | ||
| raise ValueError("Unexpected decorator format") | ||
| # extract the module and decorator names | ||
| if "CrossSync" in ast.dump(root_attr): |
Contributor
There was a problem hiding this comment.
Using ast.dump to check for the decorator's origin can be brittle as it relies on string representation. It's more robust to traverse the AST to check for the CrossSync attribute. This same pattern is used on line 258. Consider creating a helper function for this check.
- Port TLS/mTLS and experimental host support to AsyncClient - Port enable_interceptors_in_tests to AsyncInstance.database - Regenerate synchronous code via CrossSync - Fix noxfile.py for pytest-asyncio compatibility and test isolation - Add comprehensive asynchronous system tests
5d46819 to
d949d71
Compare
Contributor
|
📢 Migration Notice: 📢 This library is moving to the google-cloud-python monorepo soon. We kept this PR open due to recent activity. We would like to finalize this PR so it can be merged if it is critical. If we don't hear from the PR author, we will close this PR in the next few days. The PR can then be re-opened in the monorepo once the migration is complete and work can continue there. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description: Native Asyncio Support
This PR introduces comprehensive, native asyncio support to the google-cloud-spanner library. It transitions the library into a "Cross-Sync" architecture, where the asynchronous implementation serves as the source of truth, and the synchronous implementation is automatically kept in parity.
Key Technical Changes
Core Library Porting
Verification & Testing
9 New System Tests: Created a dedicated async system test suite (tests/system/_async/) covering:
Rich data types (Timestamp, JSON, Protobuf, etc.)
Transaction retry loops
Partitioned DML operations
Session pool lifecycle
100% Pass Rate: All new async tests and existing sync tests pass reliably against the Spanner Emulator.
Mock Server Updates: Added 40+ mock server tests to verify specific async behaviors like result-set iteration and error handling.