[SYNPY-1544] Synapse Agent OOP Model #1152

BWMac · 2025-01-08T19:49:48Z

WIP

Implementation of design doc: https://sagebionetworks.jira.com/wiki/spaces/DPE/pages/3833954306/SYNPY-1544+Synapse+Agent+OOP+Model+Mini-Design

pep8speaks · 2025-01-08T19:50:00Z

Hello @BWMac! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file synapseclient/api/agent_services.py:

Line 39:89: E501 line too long (126 > 88 characters)
Line 81:89: E501 line too long (89 > 88 characters)
Line 90:89: E501 line too long (127 > 88 characters)
Line 140:89: E501 line too long (127 > 88 characters)
Line 175:89: E501 line too long (118 > 88 characters)
Line 192:89: E501 line too long (108 > 88 characters)
Line 194:89: E501 line too long (117 > 88 characters)
Line 239:89: E501 line too long (98 > 88 characters)
Line 252:89: E501 line too long (120 > 88 characters)

In the file synapseclient/models/agent.py:

Line 69:89: E501 line too long (130 > 88 characters)
Line 72:89: E501 line too long (93 > 88 characters)
Line 74:89: E501 line too long (91 > 88 characters)
Line 78:89: E501 line too long (99 > 88 characters)
Line 83:89: E501 line too long (91 > 88 characters)
Line 141:89: E501 line too long (112 > 88 characters)
Line 163:89: E501 line too long (112 > 88 characters)
Line 187:89: E501 line too long (112 > 88 characters)
Line 217:89: E501 line too long (102 > 88 characters)
Line 219:89: E501 line too long (112 > 88 characters)
Line 262:89: E501 line too long (140 > 88 characters)
Line 277:89: E501 line too long (109 > 88 characters)
Line 292:89: E501 line too long (89 > 88 characters)
Line 312:89: E501 line too long (93 > 88 characters)
Line 320:89: E501 line too long (112 > 88 characters)
Line 340:89: E501 line too long (112 > 88 characters)
Line 353:89: E501 line too long (98 > 88 characters)
Line 367:89: E501 line too long (103 > 88 characters)
Line 385:89: E501 line too long (96 > 88 characters)
Line 398:89: E501 line too long (99 > 88 characters)
Line 414:89: E501 line too long (111 > 88 characters)
Line 417:89: E501 line too long (102 > 88 characters)
Line 419:89: E501 line too long (112 > 88 characters)
Line 442:89: E501 line too long (109 > 88 characters)

Comment last updated at 2025-01-08 21:53:48 UTC

BryanFauble · 2025-01-08T22:20:47Z

synapseclient/api/agent_services.py

+    # Request matching <https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/agent/AgentRegistrationRequest.html>
+    request = {
+        "awsAgentId": cloud_agent_id,
+        "awsAliasId": cloud_alias_id if cloud_alias_id else None,


Will this request work if you pass in None? My assumption is that the key in the http body wouldn't be sent if there is no value

BryanFauble · 2025-01-08T22:22:08Z

synapseclient/api/agent_services.py

+
+    client = Synapse.get_client(synapse_client=synapse_client)
+
+    # Request matching <https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/agent/CreateAgentSessionRequest.html>


Might make sense to put this in the docstring so it can be referenced to understand what access_level is supposed to be

BryanFauble · 2025-01-08T22:23:39Z

synapseclient/api/agent_services.py

+
+    # Request matching <https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/agent/AgentChatRequest.html>
+    request = {
+        "concreteType": "org.sagebionetworks.repo.model.agent.AgentChatRequest",


There is a constant file to throw this in to:
https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/core/constants/concrete_types.py

BryanFauble · 2025-01-08T22:32:12Z

synapseclient/api/agent_services.py

+    start_time = asyncio.get_event_loop().time()
+    TIMEOUT = 60
+
+    while True:


synapsePythonClient/synapseclient/client.py

Line 5029 in 8bd5e19

def _waitForAsync(self, uri, request, endpoint=None):

This function exists on the client, but it not built with asyncio in mind. There is some additional error handling it does for the failed state. I could see an asyncio compliant version being written and used here.

Maybe it could go in this file? https://github.com/Sage-Bionetworks/synapsePythonClient/blob/8bd5e195c389e3a83bd1308be3c6a42f25d87b8f/synapseclient/core/async_utils.py

BryanFauble · 2025-01-08T22:33:05Z

synapseclient/api/agent_services.py

+
+    Arguments:
+        prompt_id: The token of the prompt to get the trace for.
+        newer_than: The timestamp to get trace results newer than. Defaults to None (all results).


Link to the rest docs that define what format the timestamp should be in

BryanFauble · 2025-01-08T22:42:50Z

synapseclient/models/agent.py

+from synapseclient.core.async_utils import otel_trace_method
+
+
+class AgentType(Enum):


Usually I've have been using (str, Enum) types for these. Mostly so they emulate string like behavior and print to console nicely. In python 3.11 they introduced a StrEnum for this. Small ai code difference between the 2:

from enum import Enum class ColumnType(str, Enum): INTEGER = "integer" FLOAT = "float" # Behaves like a string print(ColumnType.INTEGER == "integer") # True print(len(ColumnType.INTEGER)) # 7 print(ColumnType.INTEGER.upper()) # INTEGER # JSON serialization import json print(json.dumps({"type": ColumnType.INTEGER})) # {"type": "integer"}

Vs

from enum import Enum class ColumnType(Enum): INTEGER = "integer" FLOAT = "float" # Comparison needs `.value` print(ColumnType.INTEGER == "integer") # False print(ColumnType.INTEGER.value == "integer") # True # JSON serialization needs customization import json print(json.dumps({"type": ColumnType.INTEGER.value})) # {"type": "integer"}

BryanFauble · 2025-01-08T22:46:00Z

synapseclient/models/agent.py

+    """
+    Enum representing the type of agent as defined in
+        <https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/agent/AgentType.html>
+    'BASELINE' is a default agent provided by Synapse.


Nit. Can be a list. Syntax is a empty newline before and each entry starts with -.

Might need to make sure it renders properly on my docs.

Related to ^
These will be "Less" experimental APIs, so we can add a new non-experimental section to

synapsePythonClient/mkdocs.yml

Line 55 in 8bd5e19

- API Reference:

for this. Not sure on the title, maybe "AI Agent" or something?

BryanFauble · 2025-01-08T22:48:08Z

synapseclient/models/agent.py

+        agent_registration_id: The registration ID of the agent that will be used for this session.
+        etag: The etag of the agent session.
+    """
+


Check out the tables refactor PR for examples of the syntax for an Examples section. I'd like for the docstring to have scripts (Copy/pastable) that folks could run and get started.

BryanFauble · 2025-01-08T22:50:45Z

synapseclient/models/agent.py

+        return self
+
+    @otel_trace_method(
+        method_to_trace_name=lambda self, **kwargs: f"Start_Session: {self.id}"


Will id be filled in at this point in time before a session has started?

BryanFauble · 2025-01-08T22:52:21Z

synapseclient/models/agent.py

+        Returns:
+            The retrieved AgentSession object.
+        """
+        syn = Synapse.get_client(synapse_client=synapse_client)


If you don't need to use the instance of the client you may pass the synapse_client along to get_session

BryanFauble · 2025-01-08T22:56:24Z

synapseclient/models/agent.py

+        """
+        syn = Synapse.get_client(synapse_client=synapse_client)
+
+        self.access_level = access_level


My expectation was that folks would be updating attributes on the instance and then calling the store/update functions, rather than passing the updated values into those functions.

My idea is that I wanted any arguments you pass into the function to determine how it would execute (Or data not otherwise stored on the class instance), rather than the data it would be acting on.

In short the idea being:

agent_session.access_level = new_value agent_session.update()

BryanFauble · 2025-01-08T22:59:35Z

synapseclient/models/agent.py

+    async def prompt_async(
+        self,
+        *,
+        prompt: str,


* forces things to be kwargs. It is acceptable that you may want that, but i wanted to call it out if that was not the intention.

I always want synapse_client to be a kwarg so it will never end up after a positional argument

BryanFauble · 2025-01-08T23:03:46Z

synapseclient/models/agent.py

+        session = await AgentSession(id=session_id).get_async(synapse_client=syn)
+        if session.id not in self.sessions:
+            self.sessions[session.id] = session
+        self.current_session = session.id


Rather than storing a string of the id you could just store the session object itself. Both places (list of sessions) and current_session should just have pointers to the same object instance since they are not deep copies of one another.

BryanFauble · 2025-01-08T23:05:02Z

synapseclient/models/agent.py

+    async def prompt(
+        self,
+        *,
+        session_id: Optional[str] = None,


Id think you would rather pass in a AgentSession object here

BryanFauble · 2025-01-08T23:06:12Z

synapseclient/models/agent.py

+        self,
+        *,
+        session_id: Optional[str] = None,
+        prompt: str,


Required args cannot show up after optional args. It should be fine to put this as the first argument

BryanFauble · 2025-01-08T23:07:55Z

synapseclient/models/agent.py

+    def get_chat_history(self) -> List[AgentPrompt]:
+        """Gets the chat history for the current session."""
+        # TODO: Is this the best way to do this?
+        return self.sessions[self.current_session].chat_history


With the suggestion above this could swap to

self.current_session.chat_history if self.current_session else None

https://github.com/Sage-Bionetworks/synapsePythonClient/pull/1152/files#r1907958583

BWMac added 6 commits January 6, 2025 16:24

Adds async convenience functions

7f6022e

expose convenience functions

60dc255

updates convenience functions

3fd345c

updates agent_services

9c2c6d9

removes rest_get_async exception handling

52078b3

pre-commit fixes

fbcfcef

BWMac added 4 commits January 8, 2025 14:50

delete accidentally committed script

079a02c

adds initial agent implementation

5ae3257

clean up agent

84edb76

adds missing docstrings

88c6f06

BWMac changed the title ~~WIP [SYNPY-1544] Synapse Agent OOP Model~~ [SYNPY-1544] Synapse Agent OOP Model Jan 8, 2025

pre-commit

f34e9cc

BryanFauble reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYNPY-1544] Synapse Agent OOP Model #1152

[SYNPY-1544] Synapse Agent OOP Model #1152

BWMac commented Jan 8, 2025 •

edited

Loading

pep8speaks commented Jan 8, 2025 •

edited

Loading

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025 •

edited

Loading

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025

BryanFauble Jan 8, 2025


		client = Synapse.get_client(synapse_client=synapse_client)

		# Request matching <https://rest-docs.synapse.org/rest/org/sagebionetworks/repo/model/agent/CreateAgentSessionRequest.html>

		from synapseclient.core.async_utils import otel_trace_method


		class AgentType(Enum):

[SYNPY-1544] Synapse Agent OOP Model #1152

Are you sure you want to change the base?

[SYNPY-1544] Synapse Agent OOP Model #1152

Conversation

BWMac commented Jan 8, 2025 • edited Loading

pep8speaks commented Jan 8, 2025 • edited Loading

Comment last updated at 2025-01-08 21:53:48 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanFauble Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BWMac commented Jan 8, 2025 •

edited

Loading

pep8speaks commented Jan 8, 2025 •

edited

Loading

BryanFauble Jan 8, 2025 •

edited

Loading