-
Notifications
You must be signed in to change notification settings - Fork 1
Define common code for a base class for a gRPC client #122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
packages/ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client.py
Outdated
Show resolved
Hide resolved
...ssionmanagement.v1.client/src/ni/measurementlink/sessionmanagement/v1/client/_reservation.py
Show resolved
Hide resolved
...nk.sessionmanagement.v1.client/src/ni/measurementlink/sessionmanagement/v1/client/_client.py
Outdated
Show resolved
Hide resolved
bkeryan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still reviewing...
.../ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client_base.py
Outdated
Show resolved
Hide resolved
packages/ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client.py
Outdated
Show resolved
Hide resolved
.../ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client_base.py
Outdated
Show resolved
Hide resolved
.../ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client_base.py
Outdated
Show resolved
Hide resolved
.../ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client_base.py
Outdated
Show resolved
Hide resolved
packages/ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client.py
Outdated
Show resolved
Hide resolved
...nk.sessionmanagement.v1.client/src/ni/measurementlink/sessionmanagement/v1/client/_client.py
Outdated
Show resolved
Hide resolved
...ssionmanagement.v1.client/src/ni/measurementlink/sessionmanagement/v1/client/_reservation.py
Show resolved
Hide resolved
.../ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client_base.py
Outdated
Show resolved
Hide resolved
.../ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client_base.py
Show resolved
Hide resolved
packages/ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client.py
Show resolved
Hide resolved
...ssionmanagement.v1.client/src/ni/measurementlink/sessionmanagement/v1/client/_reservation.py
Show resolved
Hide resolved
Please also make sure to run measurement-plugin-python's tests with this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors common gRPC client functionality into a base class to eliminate code duplication between client packages. It introduces a GrpcServiceClientBase class containing shared initialization and stub management logic.
- Creates a base class
GrpcServiceClientBasewith common gRPC client functionality - Refactors
SessionManagementClientandPinMapClientto inherit from the base class - Improves null-checking pattern in
_BaseSessionContainerproperty methods
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
_client_base.py (sessionmanagement) |
Introduces the common base class for gRPC service clients |
_client.py (sessionmanagement) |
Refactored to inherit from base class, removing duplicated code |
_reservation.py |
Improved null-checking patterns for better type safety |
_client_base.py (pinmap) |
Empty file - placeholder for future base class usage |
_client.py (pinmap) |
Refactored to inherit from base class, simplified implementation |
Comments suppressed due to low confidence (1)
packages/ni.measurementlink.sessionmanagement.v1.client/src/ni/measurementlink/sessionmanagement/v1/client/_client.py:88
- This entire
_get_stubmethod duplicates logic already implemented in the base class. Since the class now inherits fromGrpcServiceClientBase, this method should be removed and the base class implementation should be used instead.
def _get_stub(self) -> session_management_service_pb2_grpc.SessionManagementServiceStub:
if self._stub is None:
with self._initialization_lock:
if self._grpc_channel_pool is None:
_logger.debug("Creating unshared GrpcChannelPool.")
self._grpc_channel_pool = GrpcChannelPool()
if self._discovery_client is None:
_logger.debug("Creating unshared DiscoveryClient.")
self._discovery_client = DiscoveryClient(
grpc_channel_pool=self._grpc_channel_pool
)
if self._stub is None:
compute_nodes = self._discovery_client.enumerate_compute_nodes()
remote_compute_nodes = [node for node in compute_nodes if not node.is_local]
# Use remote node URL as deployment target if only one remote node is found.
# If more than one remote node exists, use empty string for deployment target.
first_remote_node_url = (
remote_compute_nodes[0].url if len(remote_compute_nodes) == 1 else ""
)
service_location = self._discovery_client.resolve_service(
provided_interface=GRPC_SERVICE_INTERFACE_NAME,
deployment_target=first_remote_node_url,
service_class=GRPC_SERVICE_CLASS,
)
channel = self._grpc_channel_pool.get_channel(service_location.insecure_address)
self._stub = session_management_service_pb2_grpc.SessionManagementServiceStub(
channel
)
return self._stub
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...nk.sessionmanagement.v1.client/src/ni/measurementlink/sessionmanagement/v1/client/_client.py
Outdated
Show resolved
Hide resolved
packages/ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client.py
Show resolved
Hide resolved
packages/ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client.py
Outdated
Show resolved
Hide resolved
.../ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client_base.py
Outdated
Show resolved
Hide resolved
.../ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client_base.py
Outdated
Show resolved
Hide resolved
packages/ni.measurementlink.pinmap.v1.client/src/ni/measurementlink/pinmap/v1/client/_client.py
Show resolved
Hide resolved
...nk.sessionmanagement.v1.client/src/ni/measurementlink/sessionmanagement/v1/client/_client.py
Show resolved
Hide resolved
Ran the tests in measurement-plugin-python with the two .whl files built locally. |
jfriedri-ni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later, we can put this in a new package and use the code from that single source instead
One option is to add the new base to the ni-grpc-extensions package, which has GrpcChannelPool and seems like a home with matching functionality for a gRPC client base class.
We discussed this in the 'Publishing ni-apis-python packages' Teams thread, which I don't think you're in. Adding to ni-grpc-extensions is not (an easy) option because of a circular dependency problem. We discussed a few alternatives, but didn't solidify the decision. Message me if you want more details. |
This was previously discussed. The challenge is that the base class depends on DiscoveryClient. |
What does this Pull Request accomplish?
Puts the duplicated code for a grpc client into a '_client_base.py' file which is identical and duplicated in each client package. Later, we can put this in a new package and use the code from that single source instead, but this helps identify the common code and put it where it's easy to know what is duplicated.
Apply this to sessionmanagement.v1.client and pinmap.v1.client.
Why should this Pull Request be merged?
Makes the actual _client.py files much simpler.
What testing has been done?
Straight refactor. Tests should cover it.