-
Notifications
You must be signed in to change notification settings - Fork 1.2k
initial commit for adding EMAS to adk-samples #286
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
base: main
Are you sure you want to change the base?
Conversation
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.
I did a first pass through -- generally it looks great. I have some general standards-related comments that I added to the first .py file; those apply to all files in the PR.
I'll take another look through some of the more complex Python stuff tomorrow but I wanted to get this to you in the meantime.
python/solutions/emas/.env.example
Outdated
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.
You have this file here and in the examples/stock_lookup_custom_a2a directory, but it probably doesn't need to be in both places.
```bash | ||
cp .env.example .env | ||
``` | ||
* Open the `.env` file and populate it with your specific configuration values (API keys, ports, etc.). Refer to the comments within `.env.example`. |
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.
I noticed your .env.example file has the VERTEX variables commented out. Is your recommendation that people should use the Google AI API instead?
4. **Interact:** | ||
* Click "Connect". | ||
* Click the Mic button, speak a stock request (e.g., "What is the price of Apple?"). | ||
* Click the Mic button again to stop recording. |
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.
It's a little confusing knowing whether to stop the recording or not. From experimenting it seems like you can just leave the recording running, but it would be useful to clarify how this is supposed to work.
@@ -0,0 +1,361 @@ | |||
# app/live_server.py |
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.
In general I think all the files need to have the Apache header, along with a file-level docstring.
const EventEmitter = window.EventEmitter3; | ||
|
||
// --- Assume these classes are available globally or via imports --- | ||
// --- You MUST copy the corresponding .js files from Pastra --- |
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.
Is this still true?
@@ -0,0 +1,361 @@ | |||
# app/live_server.py |
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.
Re formatting, we are trying to follow the Google OSS Python style guide. We used the black
autoformatter to take care of most formatting issues, so if you could run that over this repo that would be a good start.
Line length is one issue I think will come up.
@@ -0,0 +1,361 @@ | |||
# app/live_server.py |
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.
The Python style guide also asks for our Python code to be pylint-clean, either by fixing the issues or disabling inappropriate / inapplicable warnings using #pylint: disable=...
@@ -0,0 +1,361 @@ | |||
# app/live_server.py |
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.
Other docstrings are also important - class- and function-level.
|
||
# --- Dynamic Instruction Builder --- | ||
def get_host_agent_instruction() -> str: | ||
base_instruction = ( |
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.
""" quotes would likely be easier to read here.
logger.debug(f"A2A Task Mgr (ADK Mode): ADK Event: {event.author} - Final: {event.is_final_response()}") | ||
# We need to extract the *tool result* that the agent processed | ||
# The final response *from the ADK agent* isn't directly what the A2A server should return. | ||
if event.get_function_responses(): |
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 conditional is a bit hard to follow, just from the indenting if nothing else. Could you break
or continue
on negative conditions instead of nesting the conditions?
E.g.
if not event.get_function_response():
continue
for func_resp in event.get_function_responses():
if func_resp.name != "get_current_stock_price":
continue
etc.
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.
OK, I've done a deeper pass over the Python code. A few things here to look at.
) | ||
logger.info(f"ADK run_live started for session {session_id}") | ||
|
||
async def adk_to_client(): |
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.
Can you move these definitions (this one and the one below) out of the try ... except block, and just call them in the block instead?? That would make the logic of this function clearer, and would also let you have longer lines without running into line length constraints.
nonlocal adk_session | ||
logger.debug(f"[{session_id}] ADK -> Client task started.") | ||
audio_chunk_counter = 0 | ||
try: |
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.
If you're handling exceptions in the outer block, why do you need a try ... except block in these functions?
if not is_tool_event: | ||
logger.debug(f"[{session_id}] Skipping event: Author={event.author}, Partial={event.partial}, Content Type={type(event.content.parts[0]).__name__ if event.content and event.content.parts else 'None'}") | ||
else: | ||
if event.get_function_calls(): |
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.
Can you just add these as elif
statements on the containing block?
async def client_to_adk(): | ||
nonlocal adk_session | ||
logger.debug(f"[{session_id}] Client -> ADK task started.") | ||
try: |
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.
Same comment as above about try ... except at this level.
app.mount("/static", StaticFiles(directory=STATIC_DIR), name="static") | ||
logger.info(f"Serving static files from {STATIC_DIR}") | ||
|
||
@app.get("/") |
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.
Isn't there a way to configure a default for "GET /" that returns "/index.html", so you don't need to handle that in code? I'm sure e.g. Apache does this out of the box.
load_dotenv(dotenv_path=dotenv_path, override=True) | ||
|
||
# Import the new tool and discovery cache | ||
try: |
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.
Do you need to make this import conditional? when do you expect this import to fail?
return base_instruction | ||
|
||
# --- Agent Definition --- | ||
host_agent: Optional[Agent] = None |
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.
Why is this a global?
name="stock_info_agent", | ||
model=MODEL, | ||
description="Provides current stock price information using an external tool.", | ||
instruction="You are an assistant that provides stock prices. " |
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.
""" quotes might be more readable here.
from typing import Any, Dict, Optional | ||
|
||
|
||
class InMemoryCache: |
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.
Where is this used? I can't find it anywhere?
) | ||
return hashlib.sha256(body_str.encode()).hexdigest() | ||
|
||
class PushNotificationSenderAuth(PushNotificationAuth): |
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.
Also can't find this -- where are these classes used?
Implements "Exploring Multi-Agent Systems" (EMAS) (fka Project Horizon) architecture