chore(main): update otdf-python-proto submodule
#123
Conversation
BREAKING CHANGE: Upgrade from connect-python 0.4.2 to 0.6.0 The connect-python library underwent a major API change between versions 0.4.x and 0.6.0, requiring updates throughout the codebase. Changes: - Update dependencies: split connect-python[compiler] into separate packages - connect-python>=0.6.0,<0.7 - protoc-gen-connect-python>=0.6.0,<0.7 - Fix binary name: protoc-gen-connect_python → protoc-gen-connect-python The new version uses hyphens instead of underscores in the binary name - Regenerate proto files with new API - Deleted 15 old *_pb2_connect.py files - Generated 15 new *_connect.py files - Fixed imports to use relative paths - Update SDK implementation (kas_connect_rpc_client.py) - Replace urllib3.PoolManager with httpx.Client - Use AccessServiceClientSync instead of AccessServiceClient - Update client instantiation: base_url and session parameters - Change extra_headers parameter to headers - Add proper context manager usage for HTTP clients - Update test mocks (test_kas_client.py) - Patch httpx.Client instead of urllib3.PoolManager - Update mock setup for context managers - Fix import paths for new module structure - Update proto module exports (kas/__init__.py) - Export AccessServiceClientSync instead of AsyncAccessServiceClient
Summary of ChangesHello @b-long, 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 primarily updates the Highlights
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 successfully updates the otdf-python-proto submodule, primarily focusing on upgrading the connect-python dependency and its related tooling. The changes are comprehensive, including updates to the protobuf generation scripts, regeneration of client code, and necessary adaptations in the KAS client and its tests. The migration from urllib3 to httpx is a positive change, improving the HTTP client handling. The code modifications are consistent with the dependency upgrade and appear to be correct. I have no further suggestions.
There was a problem hiding this comment.
Code Review
This pull request updates the otdf-python-proto submodule, which involves upgrading connect-python and regenerating the protobuf client code. The changes correctly adapt the SDK to the new library version, including switching from urllib3 to httpx and updating client instantiation. The tests have also been updated accordingly. My main concern is that while the KAS service's __init__.py is updated to export the new client classes, other services' __init__.py files are not included in this PR. If they exist, they are likely broken by this change, preventing other services from being used. Additionally, there's an opportunity to improve performance by reusing httpx.Client instances across multiple requests.
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
This PR updates the otdf-python-proto submodule to use the newer connect-python version 0.6.x, migrating from version 0.4.x. The update involves regenerating protobuf client code to use the new Connect RPC library API, which changes HTTP client handling from urllib3 to httpx, updates client class names, and modifies the parameter naming convention.
Key Changes:
- Upgraded
connect-pythonfrom 0.4.x to 0.6.x and added separateprotoc-gen-connect-pythondependency - Migrated HTTP client from
urllib3.PoolManagertohttpx.Clientwith lazy initialization and resource management - Updated Connect RPC client class names (e.g.,
AccessServiceClient→AccessServiceClientSync)
Reviewed changes
Copilot reviewed 50 out of 52 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated connect-python dependency versions and added protoc-gen-connect-python |
| src/otdf_python/kas_connect_rpc_client.py | Migrated from urllib3 to httpx, added context manager support and lazy client initialization |
| src/otdf_python/kas_client.py | Added context manager methods for proper resource cleanup |
| tests/test_kas_client.py | Updated test mocks to reflect new httpx and AccessServiceClientSync usage |
| otdf-python-proto files | Regenerated protobuf connect files with new connect-python 0.6.x tooling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request updates the otdf-python-proto submodule, primarily to upgrade the connect-python dependency. The changes include regenerating the protobuf client files to align with the new version, which now uses httpx instead of urllib3. The client code in kas_client.py and kas_connect_rpc_client.py has been updated to support the new API, including adding context manager support for better resource management. The tests have also been updated accordingly. I've identified a potential resource leak issue in the backward-compatibility implementation and suggested a fix.
| def _get_http_client(self): | ||
| """Get HTTP client, creating one if needed for backward compatibility. | ||
| Returns: | ||
| httpx.Client instance | ||
| """ | ||
| if self._http_client is None: | ||
| self._http_client = self._create_http_client() | ||
| return self._http_client |
There was a problem hiding this comment.
This new _get_http_client method, intended for backward compatibility, introduces a potential resource leak. If the KASConnectRPCClient is used without a context manager and close() is not called manually, the httpx.Client instance created here will not be closed. This can lead to leaked connections and other resource issues.
The previous implementation created a new urllib3.PoolManager for each request, which, while less efficient, did not have this specific leak risk. The new approach of reusing a single httpx.Client is better for performance but requires diligent resource management.
To guide users toward the correct usage and prevent potential issues, I recommend adding a warning when the client is created outside of a context manager.
def _get_http_client(self):
"""Get HTTP client, creating one if needed for backward compatibility.
Returns:
httpx.Client instance
"""
if self._http_client is None:
logging.warning(
"KASConnectRPCClient is creating an HTTP client outside of a context manager. "
"To ensure resources are properly managed, please use the client as a context manager "
"or call .close() when finished."
)
self._http_client = self._create_http_client()
return self._http_client
No description provided.