- 
                Notifications
    You must be signed in to change notification settings 
- Fork 60
feat(memory): Add passthrough for gmdp and gmcp operations for Memory #66
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
…oryRecords operations
| logger.error("Failed to create blob event: %s", e) | ||
| raise | ||
|  | ||
| def get_event( | 
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.
Instead of adding these pass-through methods, a better way will be to directly have the attributes copied from the gmdp and gmcp_client into this client and definining the set of apis you want to import here. Something along these lines will give you all the methods.
You can then define a filter for the one you'd like to import in this client so that you aren't copying over methods for runtime/gateway etc. This will allow you to one shot import all the methods and not have this translation logic.
+    def __getattr__(self, name: str):
+        """Dynamically forward method calls to the appropriate boto3 client.
+
+        This method enables access to all boto3 client methods without explicitly
+        defining them. Methods are looked up in the following order:
+        1. gmdp_client (bedrock-agentcore) - for data plane operations
+        2. gmcp_client (bedrock-agentcore-control) - for control plane operations
+
+        Args:
+            name: The method name being accessed
+
+        Returns:
+            A callable method from the appropriate boto3 client
+
+        Raises:
+            AttributeError: If the method doesn't exist on either client
+
+        Example:
+            # Access any boto3 method directly
+            client = MemoryClient()
+
+            # These calls are forwarded to the appropriate boto3 client
+            response = client.list_memory_records(memoryId="mem-123", namespace="test")
+            metadata = client.get_memory_metadata(memoryId="mem-123")
+        """
+        # First try gmdp_client (data plane) as it's more commonly used
+        if hasattr(self.gmdp_client, name):
+            method = getattr(self.gmdp_client, name)
+            logger.debug("Forwarding method '%s' to gmdp_client", name)
+            return method
+
+        # Then try gmcp_client (control plane)
+        if hasattr(self.gmcp_client, name):
+            method = getattr(self.gmcp_client, name)
+            logger.debug("Forwarding method '%s' to gmcp_client", name)
+            return method
+
+        # Method not found on either client
+        raise AttributeError(
+            f"'{self.__class__.__name__}' object has no attribute '{name}'. "
+            f"Method not found on gmdp_client or gmcp_client. "
+            f"Available methods can be found in the boto3 documentation for "
+            f"'bedrock-agentcore' and 'bedrock-agentcore-control' services."
+        )
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.
This is a good point. Although we do lose some of the advantages of doing things like pagination under the hood for list event methods and extracting the response through returning return response['event'].
Although we can't just delete the old methods since users may have already implemented agents using those methods that now require a new .get('event') call to get the object that they previously would have needed, I agree with implementing this for all the new methods going forward that are not covered. I'll update the implementation.
…ew memory API operations
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.
LGTM
| LGTM! | 
| Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@           Coverage Diff           @@
##             main      #66   +/-   ##
=======================================
  Coverage        ?   88.35%           
=======================================
  Files           ?       17           
  Lines           ?     1606           
  Branches        ?      242           
=======================================
  Hits            ?     1419           
  Misses          ?       93           
  Partials        ?       94           
 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
| confirmed that memory tests ran and passed locally. Therefore we are merging this | 
Many of the LangChain/Crew/Llamaindex integrations require individual event management and listing of memory records in addition to just searching.
Features
__get_attr__method to the Memory Client to dynamically forward the request to either the data plane or the control plane depending on the operationTesting
Added unit tests to cover GetEvent, DeleteEvent, ListMemoryRecords, and a few other operations.