Skip to content

Commit f91e4ed

Browse files
committed
fix(py/samples): address Gemini code review feedback for dap-demo
- Restructure package: move main.py to src/dap_demo/__init__.py - Update pyproject.toml packages to use proper package directory - Update run.sh to use module execution (-m dap_demo) - Use asyncio.gather for concurrent DAP cache fetches - Improve flows to demonstrate direct tool invocation - Add heuristic-based query routing in finance and multi assistants Note: Dynamic tools from DAPs are not in the global registry, so they must be invoked directly rather than passed to ai.generate() by name.
1 parent 93f2789 commit f91e4ed

File tree

4 files changed

+335
-25
lines changed

4 files changed

+335
-25
lines changed

py/GEMINI.md

Lines changed: 194 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,7 @@
196196
with urllib.request.urlopen(url) as response: # ❌ Blocking!
197197
return response.read()
198198
199+
199200
# CORRECT - non-blocking
200201
async def fetch_data(url: str) -> bytes:
201202
async with httpx.AsyncClient() as client:
@@ -211,6 +212,7 @@
211212
with open(path) as f: # ❌ Blocking!
212213
return f.read()
213214
215+
214216
# CORRECT - non-blocking
215217
async def read_file(path: str) -> str:
216218
async with aiofiles.open(path, encoding='utf-8') as f:
@@ -233,12 +235,14 @@
233235
```python
234236
from genkit.core.http_client import get_cached_client
235237
238+
236239
# WRONG - creates new client per request (connection overhead)
237240
async def call_api(url: str) -> dict:
238241
async with httpx.AsyncClient() as client:
239242
response = await client.get(url)
240243
return response.json()
241244
245+
242246
# WRONG - stores client at init time (event loop binding issues)
243247
class MyPlugin:
244248
def __init__(self):
@@ -248,6 +252,7 @@
248252
response = await self._client.get(url) # May fail in different loop
249253
return response.json()
250254
255+
251256
# CORRECT - uses per-event-loop cached client
252257
async def call_api(url: str, token: str) -> dict:
253258
# For APIs with expiring tokens, pass auth headers per-request
@@ -258,6 +263,7 @@
258263
response = await client.get(url, headers={'Authorization': f'Bearer {token}'})
259264
return response.json()
260265
266+
261267
# CORRECT - for static auth (API keys that don't expire)
262268
async def call_api_static_auth(url: str) -> dict:
263269
client = get_cached_client(
@@ -817,9 +823,11 @@ Python-specific development and release scripts:
817823
```python
818824
from pydantic import BaseModel, Field
819825
826+
820827
class MyFlowInput(BaseModel):
821828
prompt: str = Field(default='Hello world', description='User prompt')
822829
830+
823831
@ai.flow()
824832
async def my_flow(input: MyFlowInput) -> str:
825833
return await ai.generate(prompt=input.prompt)
@@ -831,19 +839,18 @@ Python-specific development and release scripts:
831839
from typing import Annotated
832840
from pydantic import Field
833841
842+
834843
@ai.flow()
835844
async def my_flow(
836845
prompt: Annotated[str, Field(default='Hello world')] = 'Hello world',
837-
) -> str:
838-
...
846+
) -> str: ...
839847
```
840848
841849
**Wrong** (defaults won't show in Dev UI):
842850
843851
```python
844852
@ai.flow()
845-
async def my_flow(prompt: str = 'Hello world') -> str:
846-
...
853+
async def my_flow(prompt: str = 'Hello world') -> str: ...
847854
```
848855
849856
* **Sample Media URLs**: When samples need to reference an image URL (e.g., for
@@ -879,13 +886,14 @@ Python-specific development and release scripts:
879886
```python
880887
import asyncio
881888
889+
882890
async def main():
883-
# ...
884-
await asyncio.Event().wait()
891+
# ...
892+
await asyncio.Event().wait()
893+
885894
886895
# At the bottom of main.py
887896
if __name__ == '__main__':
888-
889897
ai.run_main(main())
890898
```
891899
@@ -977,6 +985,7 @@ When developing Genkit plugins, follow these additional guidelines:
977985
system: str | None = None # System prompt override
978986
metadata: dict[str, Any] | None = None # Request metadata
979987
988+
980989
# Bad: Only basic parameters
981990
class AnthropicModelConfig(BaseModel):
982991
temperature: float | None = None
@@ -995,6 +1004,7 @@ When developing Genkit plugins, follow these additional guidelines:
9951004
guardrailVersion: Version of the guardrail (default: "DRAFT").
9961005
performanceConfig: Controls latency optimization settings.
9971006
"""
1007+
9981008
guardrailIdentifier: str | None = None
9991009
guardrailVersion: str | None = None
10001010
performanceConfig: PerformanceConfiguration | None = None
@@ -1066,14 +1076,15 @@ deployment environment. This makes the code more portable and user-friendly glob
10661076
# Good: Named constant with clear purpose
10671077
DEFAULT_OLLAMA_SERVER_URL = 'http://127.0.0.1:11434'
10681078
1079+
10691080
class OllamaPlugin:
10701081
def __init__(self, server_url: str | None = None):
10711082
self.server_url = server_url or DEFAULT_OLLAMA_SERVER_URL
10721083
1084+
10731085
# Bad: Inline hardcoded value
10741086
class OllamaPlugin:
1075-
def __init__(self, server_url: str = 'http://127.0.0.1:11434'):
1076-
...
1087+
def __init__(self, server_url: str = 'http://127.0.0.1:11434'): ...
10771088
```
10781089
10791090
* **Region-Agnostic Helpers**: For cloud services with regional endpoints, provide helper
@@ -1088,20 +1099,20 @@ deployment environment. This makes the code more portable and user-friendly glob
10881099
raise ValueError('Region is required.')
10891100
# Map region to prefix...
10901101
1102+
10911103
# Bad: Hardcoded US default
1092-
def get_inference_profile_prefix(region: str = 'us-east-1') -> str:
1093-
...
1104+
def get_inference_profile_prefix(region: str = 'us-east-1') -> str: ...
10941105
```
10951106
10961107
* **Documentation Examples**: In documentation and docstrings, use placeholder values
10971108
that are clearly examples, not real values users might accidentally copy.
10981109
10991110
```python
11001111
# Good: Clear placeholder
1101-
endpoint='https://your-resource.openai.azure.com/'
1112+
endpoint = 'https://your-resource.openai.azure.com/'
11021113
11031114
# Bad: Looks like it might work
1104-
endpoint='https://eastus.api.example.com/'
1115+
endpoint = 'https://eastus.api.example.com/'
11051116
```
11061117
11071118
* **What IS Acceptable to Hardcode**:
@@ -1302,6 +1313,7 @@ plugins/{name}/tests/
13021313
```python
13031314
from unittest.mock import AsyncMock, patch
13041315
1316+
13051317
@patch('genkit.plugins.mistral.models.Mistral')
13061318
async def test_generate(mock_client_class):
13071319
mock_client = AsyncMock()
@@ -2306,6 +2318,7 @@ When mocking HTTP clients in tests, mock `get_cached_client` instead of
23062318
```python
23072319
from unittest.mock import AsyncMock, patch
23082320
2321+
23092322
@patch('my_module.get_cached_client')
23102323
async def test_api_call(mock_get_client):
23112324
mock_client = AsyncMock()
@@ -3079,3 +3092,171 @@ done
30793092
30803093
**Exception:** `bin/install_cli` intentionally omits `pipefail` as it's a user-facing
30813094
install script that handles errors differently for better user experience.
3095+
3096+
### Session Learnings (2026-02-05): DAP, ASGI Types, and Sample Structure
3097+
3098+
This session covered several important patterns for Genkit Python development.
3099+
3100+
#### Dynamic Action Provider (DAP) Best Practices
3101+
3102+
**1. DAP Tools Are NOT in the Global Registry**
3103+
3104+
Dynamic tools created via `ai.dynamic_tool()` are intentionally NOT registered in the
3105+
global registry. This means you cannot pass them to `ai.generate(tools=[...])` by name.
3106+
3107+
```python
3108+
# ❌ WRONG - dynamic tools aren't in the registry
3109+
result = await ai.generate(
3110+
prompt=query,
3111+
tools=[t.name for t in dynamic_tools], # Names won't resolve!
3112+
)
3113+
3114+
# ✅ CORRECT - invoke dynamic tools directly
3115+
tool = await my_dap.get_action('tool', 'get_weather')
3116+
result = await tool.arun(input)
3117+
```
3118+
3119+
**2. Combining Multiple DAP Tool Results**
3120+
3121+
When a query might match multiple tools, collect results instead of returning early:
3122+
3123+
```python
3124+
# ❌ WRONG - returns after first match
3125+
if tool_a and matches_a:
3126+
return await tool_a.arun(input)
3127+
if tool_b and matches_b:
3128+
return await tool_b.arun(input)
3129+
3130+
# ✅ CORRECT - collect all matching results
3131+
results: list[str] = []
3132+
if tool_a and matches_a:
3133+
results.append(str((await tool_a.arun(input)).response))
3134+
if tool_b and matches_b:
3135+
results.append(str((await tool_b.arun(input)).response))
3136+
return ' | '.join(results) if results else 'No matches'
3137+
```
3138+
3139+
**3. Use asyncio.gather for Concurrent DAP Fetches**
3140+
3141+
When fetching from multiple DAPs, use `asyncio.gather` for efficiency:
3142+
3143+
```python
3144+
# ✅ Concurrent - efficient
3145+
weather_cache, finance_cache = await asyncio.gather(
3146+
weather_dap._cache.get_or_fetch(), # noqa: SLF001
3147+
finance_dap._cache.get_or_fetch(), # noqa: SLF001
3148+
)
3149+
3150+
# ❌ Sequential - slower
3151+
weather_cache = await weather_dap._cache.get_or_fetch()
3152+
finance_cache = await finance_dap._cache.get_or_fetch()
3153+
```
3154+
3155+
#### Sample Package Structure
3156+
3157+
**pyproject.toml `packages` vs Runtime Execution**
3158+
3159+
The `[tool.hatch.build.targets.wheel].packages` setting is for **wheel building**, not
3160+
runtime execution. Samples should be run directly:
3161+
3162+
```toml
3163+
# pyproject.toml
3164+
[tool.hatch.build.targets.wheel]
3165+
packages = ["src/dap_demo"] # For wheel builds
3166+
```
3167+
3168+
```bash
3169+
# run.sh - direct file execution (NOT -m module)
3170+
uv run src/dap_demo/__init__.py "$@"
3171+
```
3172+
3173+
When using `-m` module execution, Python requires the module to be in `PYTHONPATH`.
3174+
For samples, direct file execution is simpler and matches other samples.
3175+
3176+
#### ASGI Type Compatibility
3177+
3178+
**Protocol-Based Types for Framework Portability**
3179+
3180+
Use `typing.Protocol` instead of Union types for ASGI compatibility across frameworks:
3181+
3182+
```python
3183+
# ✅ CORRECT - Protocol-based types work with any ASGI framework
3184+
from typing import Protocol, runtime_checkable
3185+
from collections.abc import Awaitable, Callable, MutableMapping
3186+
3187+
Scope = MutableMapping[str, Any]
3188+
Receive = Callable[[], Awaitable[MutableMapping[str, Any]]]
3189+
Send = Callable[[MutableMapping[str, Any]], Awaitable[None]]
3190+
3191+
3192+
@runtime_checkable
3193+
class ASGIApp(Protocol):
3194+
async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: ...
3195+
```
3196+
3197+
**Framework-Specific Middleware Uses Native Types**
3198+
3199+
When extending framework middleware classes (e.g., Litestar's `AbstractMiddleware`),
3200+
use that framework's native types, not the portable ASGI protocols:
3201+
3202+
```python
3203+
# For Litestar middleware, use litestar.types
3204+
from litestar.middleware.base import AbstractMiddleware
3205+
from litestar.types import Receive, Scope, Send # Framework-specific
3206+
3207+
3208+
class MyMiddleware(AbstractMiddleware):
3209+
async def __call__(
3210+
self,
3211+
scope: Scope, # litestar.types.Scope
3212+
receive: Receive, # litestar.types.Receive
3213+
send: Send, # litestar.types.Send
3214+
) -> None: ...
3215+
```
3216+
3217+
**Application Type Uses Any**
3218+
3219+
External frameworks define incompatible `Application` types, so use `Any`:
3220+
3221+
```python
3222+
# Intentional - frameworks have incompatible Application types
3223+
Application = Any
3224+
"""Type alias for ASGI application objects.
3225+
3226+
Note: Uses Any because external frameworks define their own ASGI types
3227+
that aren't structurally compatible with our Protocol.
3228+
"""
3229+
```
3230+
3231+
#### Optional Dependencies in Lint Configuration
3232+
3233+
For optional dependencies used only in type hints, add them to the `lint` dependency
3234+
group rather than using inline ignore comments:
3235+
3236+
```toml
3237+
# In pyproject.toml [project.optional-dependencies]
3238+
lint = [
3239+
"litestar>=2.0.0", # For web/typing.py type resolution
3240+
]
3241+
```
3242+
3243+
This allows type checkers to resolve imports during CI while keeping the package
3244+
optional for runtime.
3245+
3246+
#### Documentation Style: Avoid Section Marker Comments
3247+
3248+
Per GEMINI.md guidelines, avoid boilerplate section marker comments:
3249+
3250+
```python
3251+
# ❌ WRONG - boilerplate markers
3252+
# =============================================================================
3253+
# ASGI Protocol Types
3254+
# =============================================================================
3255+
3256+
# ✅ CORRECT - descriptive comment only
3257+
# These Protocol-based types follow the ASGI specification and are compatible
3258+
# with any ASGI framework.
3259+
```
3260+
3261+
Comments should tell **why**, not **what**. Section markers add visual noise
3262+
without adding information.

py/samples/dap-demo/pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ build-backend = "hatchling.build"
5353
requires = ["hatchling"]
5454

5555
[tool.hatch.build.targets.wheel]
56-
packages = ["src/main.py"]
56+
packages = ["src/dap_demo"]
5757

5858
[tool.hatch.metadata]
5959
allow-direct-references = true

py/samples/dap-demo/run.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,4 +56,4 @@ genkit_start_with_browser -- \
5656
-d ../../plugins \
5757
-p '*.py;*.prompt;*.json' \
5858
-R \
59-
-- uv run src/main.py "$@"
59+
-- uv run src/dap_demo/__init__.py "$@"

0 commit comments

Comments
 (0)