recipes: Add Cloud Run hosting - #2657
rorym-hudson wants to merge 1 commit into
Conversation
Migrate both recipes to ADK 2.x, add standard serving files, and prevent cloud resource initialization during module import.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| EXPOSE 8080 | ||
|
|
||
| CMD ["uv", "run", "uvicorn", "cyber_guardian.fast_api_app:app", "--host", "0.0.0.0", "--port", "8080"] |
There was a problem hiding this comment.
[MAJOR] Can you please add a USER directive to avoid running the container as root? Running as non-root is a security best practice.
|
|
||
| EXPOSE 8080 | ||
|
|
||
| CMD ["uv", "run", "uvicorn", "high_volume_document_analyzer.fast_api_app:app", "--host", "0.0.0.0", "--port", "8080"] |
There was a problem hiding this comment.
[MAJOR] missing USER directive here
| ``APP_URL``). Call once per app — typically in a FastAPI ``lifespan``, since | ||
| the card is built asynchronously; repeated calls register duplicate routes. | ||
| """ | ||
| resolved_app_url = app_url or os.getenv("APP_URL", "http://0.0.0.0:8000") |
There was a problem hiding this comment.
[MAJOR] Is there a reason we have hardcoded defaults on environment reads here? There are six such occurrences in this PR where defaults should be defined in .env.example instead.
| from cyber_guardian.app_utils import services | ||
| from cyber_guardian.app_utils.a2a import attach_a2a_routes | ||
|
|
||
| load_dotenv() |
There was a problem hiding this comment.
[MAJOR] Perhaps we should move the load_dotenv() call to cyber_guardian/__init__.py? The handbook indicates it belongs in the package initialization module.
| from high_volume_document_analyzer.app_utils import services | ||
| from high_volume_document_analyzer.app_utils.a2a import attach_a2a_routes | ||
|
|
||
| load_dotenv() |
There was a problem hiding this comment.
[MAJOR] load_dotenv() belongs in __init__.py
| allowed = streaming_methods if streaming else sync_methods | ||
| if class_method not in allowed: | ||
| raise HTTPException( | ||
| status_code=404, |
There was a problem hiding this comment.
[MINOR] Is there a reason we are not using standard HTTP status constants here? We can use status.HTTP_404_NOT_FOUND from fastapi instead of the bare 404 literal across the two reasoning engine adapters.
| logger = logging.getLogger(__name__) | ||
|
|
||
| root_agent = Agent( | ||
| root_agent = LlmAgent( |
There was a problem hiding this comment.
[MINOR] It seems cyber_guardian/agent.py is missing the app definition. Can we import App from google.adk.apps and define app = App(root_agent=root_agent, name="cyber_guardian") as done in high_volume_document_analyzer/agent.py?
|
|
||
| @contextlib.asynccontextmanager | ||
| async def lifespan(app: FastAPI) -> AsyncIterator[None]: | ||
| from cyber_guardian.agent import app as adk_app |
There was a problem hiding this comment.
[MAJOR] It seems cyber_guardian/agent.py is missing the instantiation of app = App(root_agent=root_agent, name="cyber_guardian"). Can you please define it there so this import does not fail?
| ) | ||
| app.state.runner = runner | ||
| app.state.agent_app_name = adk_app.name | ||
| await attach_a2a_routes( |
There was a problem hiding this comment.
[MAJOR] Can you please import and call attach_reasoning_engine_routes(app) here? Otherwise, the reasoning engine routes will never be registered on the FastAPI application.
| ) | ||
| app.state.runner = runner | ||
| app.state.agent_app_name = adk_app.name | ||
| await attach_a2a_routes( |
There was a problem hiding this comment.
[MAJOR] same issue as above
|
|
||
| # 0.3 uses method names that include a '/' like "message/send" | ||
| # 1.0 uses PascalCase like "SendMessage" | ||
| json_body = getattr(request, "_json", {}) or {} |
There was a problem hiding this comment.
[MAJOR] Is there a reason we are accessing the private _json attribute on the request object? This is fragile and will be empty if the JSON body has not yet been parsed.
|
|
||
| # 0.3 uses method names that include a '/' like "message/send" | ||
| # 1.0 uses PascalCase like "SendMessage" | ||
| json_body = getattr(request, "_json", {}) or {} |
There was a problem hiding this comment.
[MAJOR] same issue as above
happyhuman
left a comment
happyhuman
left a comment
There was a problem hiding this comment.
We are no longer accepting any futher contribution in recipes under python/agents and we are migrating them to contrib/python.
Please move your recipe to that folder, and upgrade it to meet the new quality standards of the repo. Please see docs/ for more information.
2 participants
Summary
Validation