Optimize delays - realtime, waiting logic and error handling #132
Conversation
WalkthroughThe PR introduces profiling instrumentation to the vision agents framework via a new Profiler class, adds AgentInitEvent and AgentFinishEvent for agent lifecycle management, updates the event manager with synchronization primitives, refactors the Gemini LLM plugin to use async clients with multi-hop tool calling, and instruments examples with profiling capabilities. Configuration files are updated with pyinstrument dependencies, and profiling artifacts are added to .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Agent
participant Profiler
participant EventManager
participant Handlers
App->>+Agent: __init__(profiler=Profiler())
Agent->>+EventManager: register profiler events
Agent->>-EventManager: emit AgentInitEvent
Note over EventManager: AgentInitEvent queued & processed
App->>Agent: run agent workflow
Agent->>Agent: execute tasks...
Agent->>+EventManager: CallEndedEvent detected
EventManager->>EventManager: signal _received_event
EventManager->>-Agent: unblock finish()
Agent->>+EventManager: emit AgentFinishEvent
Note over EventManager: AgentFinishEvent queued & processed
EventManager->>+Profiler: invoke on_finish handler
Profiler->>Profiler: stop pyinstrument profiler
Profiler->>Profiler: write profile.html
Profiler->>-EventManager: handler completes
EventManager->>-Agent: finish
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested Reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yarikdevcom
force-pushed
the
profiling/realtime-data
branch
from
cbf9efc to
69ac841
Compare
yarikdevcom
changed the title
yarikdevcom
force-pushed
the
profiling/realtime-data
branch
from
35eedab to
29b7a8a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py (1)
152-158: Apply async/await to follow-up tool call streaming at lines 152 and 158.Line 102 establishes the correct pattern:
await self.chat.send_message_stream()followed byasync for. Line 152 omits theawait, leavingfollow_up_iteras an unawaited coroutine, then line 158 attempts synchronous iteration, which would fail at runtime.- follow_up_iter = self.chat.send_message_stream(parts, config=cfg_with_tools) # type: ignore[arg-type] + follow_up_iter = await self.chat.send_message_stream(parts, config=cfg_with_tools) # type: ignore[arg-type] follow_up_text_parts: List[str] = [] follow_up_last = None next_calls = [] - for idx, chk in enumerate(follow_up_iter): + async for idx, chk in enumerate(follow_up_iter):Note:
enumerate()supports async iteration, so no manual index tracking is needed.agents-core/vision_agents/core/agents/agents.py (1)
544-567: Fix finish() hang when CallEndedEvent already firedSubscribing to
CallEndedEventinsidefinish()means we miss any event that fired earlier. If the remote participant drops before we callawait agent.finish(), the event manager never replays that notification,running_eventstays unset, and this coroutine blocks forever. Please handle the “call already ended” path—e.g., set the event immediately when you detect the connection is no longer active or register the handler earlier so the flag is updated beforefinish()waits.
🧹 Nitpick comments (3)
plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py (1)
117-121: Consider logging ignored extraction errors for debugging.The bare
except: passblocks silence all errors during tool call extraction. While this prevents crashes from malformed chunks, it may hide legitimate issues during development or debugging.Consider adding debug-level logging:
try: chunk_calls = self._extract_tool_calls_from_stream_chunk(chunk) pending_calls.extend(chunk_calls) - except Exception: - pass # Ignore errors in chunk processing + except Exception as e: + logger.debug(f"Ignoring tool call extraction error: {e}")Apply the same pattern at lines 163-168.
Also applies to: 163-168
agents-core/vision_agents/core/agents/events.py (2)
6-11: Well-structured lifecycle event.
AgentInitEventcorrectly inherits fromBaseEvent(rather thanPluginBaseEvent) since this is an agent lifecycle marker, not a plugin-specific operation. Theinit=Falseparameter on the type field appropriately prevents callers from overriding the event type.The docstring could be expanded to clarify the emission context:
- """Event emitted when Agent class initialized.""" + """Event emitted when an Agent instance is initialized. + + This event marks the beginning of an agent's lifecycle and is used + for profiling and tracking agent initialization. + """
13-18: Lifecycle event properly implemented.
AgentFinishEventfollows the same pattern asAgentInitEventand correctly usesBaseEventas the parent class. The structure is clean and consistent.Consider enriching the docstring to provide context about the finish phase:
- """Event emitted when agent.finish() call ended.""" + """Event emitted when an agent's finish() method completes. + + This event marks the end of an agent's lifecycle and is used for + profiling, cleanup tracking, and resource finalization monitoring. + """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
examples/01_simple_agent_example/uv.lockis excluded by!**/*.lockexamples/other_examples/openai_realtime_webrtc/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.gitignore(1 hunks)agents-core/vision_agents/core/agents/agents.py(6 hunks)agents-core/vision_agents/core/agents/events.py(1 hunks)agents-core/vision_agents/core/events/manager.py(7 hunks)agents-core/vision_agents/core/profiling/__init__.py(1 hunks)agents-core/vision_agents/core/profiling/base.py(1 hunks)examples/01_simple_agent_example/pyproject.toml(3 hunks)examples/01_simple_agent_example/simple_agent_example.py(2 hunks)examples/other_examples/openai_realtime_webrtc/openai_realtime_example.py(3 hunks)examples/other_examples/openai_realtime_webrtc/pyproject.toml(1 hunks)plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py(5 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
examples/01_simple_agent_example/simple_agent_example.pyexamples/other_examples/openai_realtime_webrtc/openai_realtime_example.pyplugins/gemini/vision_agents/plugins/gemini/gemini_llm.pyagents-core/vision_agents/core/agents/events.pyagents-core/vision_agents/core/profiling/__init__.pyagents-core/vision_agents/core/agents/agents.pyagents-core/vision_agents/core/events/manager.pyagents-core/vision_agents/core/profiling/base.py
🧠 Learnings (1)
📚 Learning: 2025-10-13T22:00:34.300Z
Learnt from: dangusev
Repo: GetStream/Vision-Agents PR: 98
File: plugins/deepgram/vision_agents/plugins/deepgram/stt.py:135-150
Timestamp: 2025-10-13T22:00:34.300Z
Learning: In the Deepgram STT plugin (plugins/deepgram/vision_agents/plugins/deepgram/stt.py), the `started()` method is designed to wait for the connection attempt to complete, not to guarantee a successful connection. It's acceptable for the connection attempt to fail, and downstream code handles the case where `self.dg_connection` is `None`. The `_connected_once` event is set in the `finally` block intentionally to signal attempt completion.
Applied to files:
agents-core/vision_agents/core/agents/agents.py
🧬 Code graph analysis (8)
examples/01_simple_agent_example/simple_agent_example.py (2)
agents-core/vision_agents/core/profiling/base.py (1)
Profiler(10-23)plugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.py (1)
TTS(12-74)
examples/other_examples/openai_realtime_webrtc/openai_realtime_example.py (1)
agents-core/vision_agents/core/profiling/base.py (1)
Profiler(10-23)
plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py (1)
agents-core/vision_agents/core/llm/llm_types.py (1)
NormalizedToolCallItem(107-111)
agents-core/vision_agents/core/agents/events.py (1)
agents-core/vision_agents/core/events/base.py (2)
PluginBaseEvent(52-54)BaseEvent(34-48)
agents-core/vision_agents/core/profiling/__init__.py (1)
agents-core/vision_agents/core/profiling/base.py (1)
Profiler(10-23)
agents-core/vision_agents/core/agents/agents.py (3)
agents-core/vision_agents/core/profiling/base.py (1)
Profiler(10-23)agents-core/vision_agents/core/events/manager.py (2)
send(428-472)wait(474-488)agents-core/vision_agents/core/agents/events.py (2)
AgentInitEvent(7-10)AgentFinishEvent(14-17)
agents-core/vision_agents/core/events/manager.py (1)
agents-core/vision_agents/core/events/base.py (1)
ExceptionEvent(97-100)
agents-core/vision_agents/core/profiling/base.py (3)
agents-core/vision_agents/core/events/manager.py (3)
EventManager(56-551)register_events_from_module(219-256)subscribe(301-370)agents-core/vision_agents/core/agents/agents.py (1)
subscribe(285-297)agents-core/vision_agents/core/agents/events.py (1)
AgentFinishEvent(14-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Ruff & mypy
- GitHub Check: unit / Test "not integration"
🔇 Additional comments (14)
.gitignore (1)
88-90: Clarify the purpose of opencode.json.The
profile.htmlentry aligns with the profiling infrastructure introduced in this PR. However,opencode.jsonlacks context in the PR description and related changes.Could you clarify what
opencode.jsonis used for and why it's being ignored at the repository root?examples/other_examples/openai_realtime_webrtc/pyproject.toml (1)
13-13: LGTM!The pyinstrument dependency addition aligns with the profiling infrastructure introduced in this PR.
pyproject.toml (1)
83-83: LGTM!Adding pyinstrument as a dev dependency is appropriate for the profiling infrastructure introduced in this PR.
examples/01_simple_agent_example/pyproject.toml (1)
6-6: LGTM!The addition of the vogent plugin and pyinstrument dependency, along with the corresponding source configuration, aligns with the profiling and plugin enhancements in this PR.
Also applies to: 17-17, 23-23, 35-35
agents-core/vision_agents/core/events/manager.py (5)
145-145: LGTM!Introducing
_received_eventfor internal synchronization is a solid improvement over sleep-based polling in event processing.
217-217: LGTM!Sharing
_received_eventduring merge ensures proper synchronization across merged event managers.
472-472: LGTM!Setting
_received_eventafter queueing ensures the processing loop wakes up promptly to handle new events.
528-530: LGTM!Replacing the sleep with
await _received_event.wait()is a more efficient approach that eliminates unnecessary delays while ensuring the loop resumes promptly when events arrive.
536-536: LGTM!Routing exceptions through the standard
send()method ensures they're processed consistently with other events and properly signals the event loop.plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py (4)
4-4: LGTM!The migration to
AsyncClientand use of.aioaccessor is consistent with the async pattern adopted in this PR.Also applies to: 40-40, 57-57
102-102: LGTM!The async streaming implementation with
awaitandasync foris correct, and theidxtracking properly annotates emitted events.Also applies to: 110-123
126-137: LGTM!The multi-round tool calling setup with
MAX_ROUNDS = 3, deduplication, and concurrent execution with timeout is well-designed to handle complex tool interactions safely.
138-150: LGTM!The concurrent tool execution with result sanitization and proper conversion to Gemini's function response format is well-implemented.
agents-core/vision_agents/core/agents/events.py (1)
2-2: LGTM! Import updated correctly.The addition of
BaseEventto the imports is necessary for the new agent lifecycle events and follows the existing import pattern.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py (1)
152-169: Fix the follow-up stream handling
chat.send_message_streamreturns an awaitable async iterator. Here we neither await it nor iterate withasync for, so we hand a coroutine toenumerate, tripping mypy and blowing up at runtime once tool calls trigger this path. The Google GenAI SDK examples requireasync for chunk in await chat.send_message_stream(...)to consume the stream(googleapis.github.io). Please await the call and switch to an async loop with a manual index, e.g.:- follow_up_iter = self.chat.send_message_stream(parts, config=cfg_with_tools) # type: ignore[arg-type] - - follow_up_text_parts: List[str] = [] - follow_up_last = None - next_calls = [] - - for idx, chk in enumerate(follow_up_iter): + follow_up_iter = await self.chat.send_message_stream(parts, config=cfg_with_tools) # type: ignore[arg-type] + + follow_up_text_parts: List[str] = [] + follow_up_last = None + next_calls = [] + follow_idx = 0 + + async for chk in follow_up_iter: follow_up_last = chk # TODO: unclear if this is correct (item_id and idx) - self._standardize_and_emit_event(chk, follow_up_text_parts, item_id, idx) + self._standardize_and_emit_event(chk, follow_up_text_parts, item_id, follow_idx) + follow_idx += 1agents-core/vision_agents/core/agents/agents.py (1)
544-566: Add timeout and improve error handling in finish().The refactored
finish()method has several concerns:
Missing timeout: Line 560's
await running_event.wait()has no timeout. IfCallEndedEventis never emitted (e.g., due to network issues or edge transport bugs), this will hang indefinitely. Consider adding a timeout parameter or a reasonable default.Subscription cleanup: The
on_endedsubscription (line 553) is never unsubscribed. If the function exits early or is called multiple times, old subscriptions will accumulate.CancelledError handling: Lines 561-563 catch
CancelledError, clear the event, but then continue to line 564 to sendAgentFinishEventand callclose(). Is it intentional to emit the finish event and close even when cancelled? This might be correct, but should be documented.Race condition: Between lines 544 and 553, there's a window where the event could fire before the subscription is registered if
CallEndedEventwas already queued.Consider this refactor:
async def finish(self): """Wait for the call to end gracefully. Subscribes to the edge transport's `call_ended` event and awaits it. If no connection is active, returns immediately. """ if not self._connection: self.logger.info( "🔚 Agent connection is already closed, finishing immediately" ) return - running_event = asyncio.Event() + running_event = asyncio.Event() + subscription = None + with self.span("agent.finish"): # If connection is None or already closed, return immediately if not self._connection: logging.info( "🔚 Agent connection already closed, finishing immediately" ) return @self.edge.events.subscribe async def on_ended(event: CallEndedEvent): running_event.set() self._is_running = False + + subscription = on_ended - # TODO: add members count check (particiapnts left + count = 1 timeout 2 minutes) + # TODO: add members count check (participants left + count = 1 timeout 2 minutes) try: - await running_event.wait() + await asyncio.wait_for(running_event.wait(), timeout=120.0) except asyncio.CancelledError: running_event.clear() + raise # Re-raise to prevent finish event emission when cancelled + except asyncio.TimeoutError: + self.logger.warning("finish() timed out waiting for CallEndedEvent") + finally: + # Clean up subscription + if subscription and hasattr(self.edge.events, 'unsubscribe'): + self.edge.events.unsubscribe(subscription) self.events.send(events.AgentFinishEvent()) await asyncio.shield(self.close())
🧹 Nitpick comments (1)
examples/other_examples/openai_realtime_webrtc/openai_realtime_example.py (1)
14-14: Consider using the Profiler class for consistency.The manual pyinstrument setup duplicates functionality available in
vision_agents.core.profiling.Profiler. For consistency with other examples (e.g.,simple_agent_example.py), consider using the Profiler class which handles start/stop lifecycle automatically viaAgentFinishEvent.If the agent setup supports it, replace manual profiling with:
-import pyinstrument +from vision_agents.core.profiling import Profiler async def start_agent() -> None: - profiler = pyinstrument.Profiler() - profiler.start() # ... agent = Agent( # ... other params + profiler=Profiler(output_path='profiled.html'), ) # ... await agent.finish() - - profiler.stop() - with open('profiled.html', 'w') as f: - f.write(profiler.output_html())Also applies to: 27-28, 78-80
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
examples/01_simple_agent_example/uv.lockis excluded by!**/*.lockexamples/other_examples/openai_realtime_webrtc/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.gitignore(1 hunks)agents-core/vision_agents/core/agents/agents.py(6 hunks)agents-core/vision_agents/core/agents/events.py(1 hunks)agents-core/vision_agents/core/events/manager.py(7 hunks)agents-core/vision_agents/core/profiling/__init__.py(1 hunks)agents-core/vision_agents/core/profiling/base.py(1 hunks)examples/01_simple_agent_example/pyproject.toml(3 hunks)examples/01_simple_agent_example/simple_agent_example.py(2 hunks)examples/other_examples/openai_realtime_webrtc/openai_realtime_example.py(3 hunks)examples/other_examples/openai_realtime_webrtc/pyproject.toml(1 hunks)plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py(5 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Do not modify sys.path in Python code
Docstrings must follow the Google style guide
Files:
agents-core/vision_agents/core/profiling/__init__.pyagents-core/vision_agents/core/profiling/base.pyexamples/other_examples/openai_realtime_webrtc/openai_realtime_example.pyagents-core/vision_agents/core/agents/events.pyplugins/gemini/vision_agents/plugins/gemini/gemini_llm.pyagents-core/vision_agents/core/agents/agents.pyexamples/01_simple_agent_example/simple_agent_example.pyagents-core/vision_agents/core/events/manager.py
🧠 Learnings (1)
📚 Learning: 2025-10-13T22:00:34.300Z
Learnt from: dangusev
Repo: GetStream/Vision-Agents PR: 98
File: plugins/deepgram/vision_agents/plugins/deepgram/stt.py:135-150
Timestamp: 2025-10-13T22:00:34.300Z
Learning: In the Deepgram STT plugin (plugins/deepgram/vision_agents/plugins/deepgram/stt.py), the `started()` method is designed to wait for the connection attempt to complete, not to guarantee a successful connection. It's acceptable for the connection attempt to fail, and downstream code handles the case where `self.dg_connection` is `None`. The `_connected_once` event is set in the `finally` block intentionally to signal attempt completion.
Applied to files:
agents-core/vision_agents/core/agents/agents.py
🧬 Code graph analysis (8)
agents-core/vision_agents/core/profiling/__init__.py (1)
agents-core/vision_agents/core/profiling/base.py (1)
Profiler(10-23)
agents-core/vision_agents/core/profiling/base.py (3)
agents-core/vision_agents/core/events/manager.py (2)
register_events_from_module(219-256)subscribe(301-370)agents-core/vision_agents/core/agents/agents.py (1)
subscribe(285-297)agents-core/vision_agents/core/agents/events.py (1)
AgentFinishEvent(14-17)
examples/other_examples/openai_realtime_webrtc/openai_realtime_example.py (1)
agents-core/vision_agents/core/profiling/base.py (1)
Profiler(10-23)
agents-core/vision_agents/core/agents/events.py (1)
agents-core/vision_agents/core/events/base.py (2)
PluginBaseEvent(52-54)BaseEvent(34-48)
plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py (1)
agents-core/vision_agents/core/llm/llm_types.py (1)
NormalizedToolCallItem(107-111)
agents-core/vision_agents/core/agents/agents.py (3)
agents-core/vision_agents/core/profiling/base.py (1)
Profiler(10-23)agents-core/vision_agents/core/events/manager.py (2)
send(428-472)wait(474-488)agents-core/vision_agents/core/agents/events.py (2)
AgentInitEvent(7-10)AgentFinishEvent(14-17)
examples/01_simple_agent_example/simple_agent_example.py (2)
agents-core/vision_agents/core/profiling/base.py (1)
Profiler(10-23)plugins/elevenlabs/vision_agents/plugins/elevenlabs/tts.py (1)
TTS(12-74)
agents-core/vision_agents/core/events/manager.py (1)
agents-core/vision_agents/core/events/base.py (1)
ExceptionEvent(97-100)
🪛 GitHub Actions: CI (unit)
plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py
[error] 158-158: Mypy type-check error: Argument 1 to 'enumerate' has incompatible type 'Coroutine[Any, Any, AsyncIterator[GenerateContentResponse]] | Any'; expected 'Iterable[Any]'.
🔇 Additional comments (15)
.gitignore (1)
88-90: Appropriate additions for profiling and tooling artifacts.Both entries align well with the PR objectives—
profile.htmlcaptures output from the new profiling instrumentation, while/opencode.jsonexcludes root-level code-generation artifacts. The placement under "Artifacts / assets" is sensible, and the use of a leading slash for/opencode.jsonensures proper root-level scoping.examples/other_examples/openai_realtime_webrtc/pyproject.toml (1)
13-13: Dependency addition looks good.The pyinstrument dependency supports the profiling instrumentation added in the example code.
pyproject.toml (1)
83-83: Dev dependency addition is appropriate.Adding pyinstrument to dev dependencies enables profiling across the workspace.
examples/01_simple_agent_example/pyproject.toml (1)
6-6: Dependencies and comment update look good.The changes appropriately add vogent plugin and pyinstrument dependencies, with proper source mappings following the existing pattern.
Also applies to: 17-17, 23-23, 35-35
agents-core/vision_agents/core/events/manager.py (3)
145-145: Event-driven synchronization improves performance.Replacing the polling sleep with
asyncio.Eventprovides better responsiveness and reduces unnecessary wake-ups. The propagation inmergeensures consistency across merged managers.Also applies to: 217-217, 472-472, 528-530
412-412: Logging level adjustments are appropriate.Moving high-frequency event logs from info to debug reduces noise while maintaining observability for debugging.
Also applies to: 545-547
536-536: Revert this review comment — ExceptionEvent is properly handled and will not be dropped.The concern assumes
_prepare_event()could returnNoneforExceptionEvent, but analysis shows this cannot occur.ExceptionEventis registered during initialization (line 147) and has atypeattribute set to"base.exception". When_prepare_event()is called at line 468, it reaches the final validation (line 408) which checks ifevent.type in self._events. SinceExceptionEventis registered, this check passes and the method explicitly returns the event at line 410-411. The event is then successfully appended to the queue at line 470. No code path exists that would silently dropExceptionEvent.Likely an incorrect or invalid review comment.
examples/01_simple_agent_example/simple_agent_example.py (2)
7-7: Profiler integration looks good.The Profiler usage follows the correct pattern, automatically handling lifecycle via
AgentFinishEventsubscription.Also applies to: 27-27
6-6: I need to verify that ElevenLabs is properly imported in the modified file:Confirm TTS provider change from Cartesia to ElevenLabs is intentional.
The codebase shows Cartesia remains actively maintained and used in other examples (e.g.,
plugins/sample_plugin/example/my_example.py, AWS examples,tts_cartesia/example directory), while ElevenLabs is an established, production-ready TTS integration in the framework with support for configurable models and voice IDs. This appears to be a deliberate choice to demonstrate ElevenLabs TTS in this specific example rather than a complete migration. The change is appropriate, and no action is required.agents-core/vision_agents/core/profiling/__init__.py (1)
1-3: Clean public API exposure.The module initialization properly exports the Profiler class, following Python packaging conventions.
agents-core/vision_agents/core/agents/events.py (1)
2-2: Agent lifecycle events are well-defined.The
AgentInitEventandAgentFinishEventclasses follow the established event pattern and provide clear lifecycle hooks for profiling and other observability features.Also applies to: 6-17
agents-core/vision_agents/core/agents/agents.py (4)
54-54: LGTM!The profiler import is clean and follows the project's relative import pattern.
213-216: LGTM!The profiler is correctly integrated into the plugin aggregation loop with proper None-safety checks. The conditional on line 214 ensures that only plugins with event managers are merged.
243-244: Verify event subscribers are ready for AgentInitEvent.The
AgentInitEventis emitted at the end of__init__, but event handler subscriptions are registered later in thejoin()method (lines 478-479). This means:
- The profiler's
on_finishhandler is subscribed in the Profiler's__init__(line 17 of base.py), so it should receive events- Other handlers registered in
_setup_llm_events()and_setup_speech_events()won't be active yetEnsure that all intended subscribers to
AgentInitEventare registered before this event is sent, or document that this event is only for early-stage subscribers like the profiler.
158-158: Clarify whether log_level default was actually changed from INFO to DEBUG.The review comment asserts that "the default log level has been changed from INFO to DEBUG," but I found no evidence supporting this claimed transition:
- No git history for the
log_levelparameter- No test files document prior default values
- No documentation or README shows prior configuration
- The Agent class docstring example does not mention
log_levelusageThe parameter currently defaults to
logging.DEBUGat line 158, andconfigure_default_logging()applies it to handlers. However, without verifiable evidence of a prior INFO default, the factual claim of a "change" cannot be confirmed.The underlying concern about DEBUG-level logging producing excessive verbosity is worth addressing, but verify first whether this is a newly added parameter or an existing one that was always DEBUG.
Likely an incorrect or invalid review comment.
| class Profiler: | ||
| def __init__(self, output_path='./profile.html'): | ||
| self.output_path = output_path | ||
| self.events = EventManager() | ||
| self.events.register_events_from_module(events) | ||
| self.profiler = pyinstrument.Profiler() | ||
| self.profiler.start() | ||
| self.events.subscribe(self.on_finish) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add docstrings and improve profiler lifecycle management.
Multiple concerns with the Profiler class:
-
Missing docstrings: The class and
__init__method lack docstrings. As per coding guidelines, docstrings must follow the Google style guide. -
Premature profiling start: The profiler starts immediately on line 16, even before the agent joins a call. This captures initialization overhead that may not be relevant to profiling agent behavior during a call. Consider starting the profiler when
AgentInitEventis received instead. -
No cleanup mechanism: If
AgentFinishEventis never emitted (e.g., iffinish()is never called or the process crashes), the profiler runs indefinitely and the HTML file is never written. Consider:- Adding a context manager interface (
__enter__/__exit__) - Adding an explicit
stop()method for manual cleanup - Adding a destructor (
__del__) as a fallback
- Adding a context manager interface (
-
No output path validation: Line 11 accepts any path without validation. If the directory doesn't exist or isn't writable, the error won't surface until
on_finish.
Here's a proposed refactor:
class Profiler:
+ """Profile agent lifecycle and generate HTML profiling reports.
+
+ Automatically starts profiling when AgentInitEvent is emitted and stops
+ when AgentFinishEvent is emitted, writing results to the specified path.
+
+ Args:
+ output_path: Path where the HTML profiling report will be written.
+ Defaults to './profile.html'.
+
+ Example:
+ profiler = Profiler(output_path='./my_profile.html')
+ agent = Agent(..., profiler=profiler)
+ # Profiler automatically captures agent lifecycle
+ """
+
def __init__(self, output_path='./profile.html'):
+ """Initialize the profiler.
+
+ Args:
+ output_path: Path where the HTML profiling report will be written.
+ """
self.output_path = output_path
self.events = EventManager()
self.events.register_events_from_module(events)
self.profiler = pyinstrument.Profiler()
- self.profiler.start()
+ self._started = False
+ self.events.subscribe(self.on_init)
self.events.subscribe(self.on_finish)
+
+ async def on_init(self, event: events.AgentInitEvent):
+ """Start profiling when agent initializes."""
+ if not self._started:
+ self.profiler.start()
+ self._started = True
+ logger.info("Profiler started")
async def on_finish(self, event: events.AgentFinishEvent):
+ """Stop profiling and write results when agent finishes."""
+ if not self._started:
+ logger.warning("Profiler was never started, skipping output")
+ return
self.profiler.stop()
logger.info(f"Profiler stopped. Time file saved at: {self.output_path}")
with open(self.output_path, 'w') as f:
f.write(self.profiler.output_html())
+
+ def stop(self):
+ """Manually stop profiling and write results (if not already stopped)."""
+ if self._started and self.profiler:
+ self.profiler.stop()
+ self._started = False
+ logger.info(f"Profiler stopped manually. Writing to: {self.output_path}")
+ with open(self.output_path, 'w') as f:
+ f.write(self.profiler.output_html())
+
+ def __del__(self):
+ """Ensure profiler is stopped on garbage collection."""
+ try:
+ if self._started:
+ self.stop()
+ except Exception:
+ pass # Avoid exceptions in __del__🤖 Prompt for AI Agents
In agents-core/vision_agents/core/profiling/base.py around lines 10 to 18, add
Google-style docstrings for the Profiler class and its __init__; remove
immediate self.profiler.start() from __init__ and instead start profiling when
an AgentInitEvent is received (subscribe to AgentInitEvent and call start
there), and stop/profile dump on AgentFinishEvent; add explicit start() and
stop() methods, implement __enter__/__exit__ to support context-manager usage,
and add a __del__ fallback that calls stop(); validate output_path in __init__
(ensure parent directory exists or create it and verify writability, raising a
clear exception if invalid) so on_finish can always write the HTML; ensure
on_finish unsubscribes and writes the profiler output safely and that stop() is
idempotent.
| async def on_finish(self, event: events.AgentFinishEvent): | ||
| self.profiler.stop() | ||
| logger.info(f"Profiler stopped. Time file saved at: {self.output_path}") | ||
| with open(self.output_path, 'w') as f: | ||
| f.write(self.profiler.output_html()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling and improve async file I/O.
The on_finish method has several issues:
-
Missing docstring: As per coding guidelines, docstrings must follow the Google style guide.
-
No error handling: Lines 22-23 perform file I/O without error handling. If the directory doesn't exist, the path is invalid, or there are permission issues, the exception will propagate and potentially crash the event handler. Wrap file operations in try-except.
-
Synchronous I/O in async function: Using synchronous
open()in an async function can block the event loop. Consider usingaiofilesfor async file I/O. -
Missing encoding specification: HTML files should explicitly specify encoding (UTF-8) to avoid potential encoding issues across platforms.
-
No validation: Line 20 calls
stop()without checking if the profiler was started. Ifon_finishis called twice, this could raise an exception.
Apply this diff:
async def on_finish(self, event: events.AgentFinishEvent):
+ """Stop profiling and write HTML report when agent finishes.
+
+ Args:
+ event: The AgentFinishEvent triggering profiler shutdown.
+ """
+ try:
- self.profiler.stop()
- logger.info(f"Profiler stopped. Time file saved at: {self.output_path}")
- with open(self.output_path, 'w') as f:
- f.write(self.profiler.output_html())
+ self.profiler.stop()
+ html_output = self.profiler.output_html()
+
+ # Consider using aiofiles for async I/O
+ with open(self.output_path, 'w', encoding='utf-8') as f:
+ f.write(html_output)
+
+ logger.info(f"Profiler stopped. Profile saved at: {self.output_path}")
+ except Exception as e:
+ logger.error(f"Failed to write profiler output to {self.output_path}: {e}", exc_info=True)Or, for fully async I/O:
import aiofiles
async def on_finish(self, event: events.AgentFinishEvent):
"""Stop profiling and write HTML report when agent finishes.
Args:
event: The AgentFinishEvent triggering profiler shutdown.
"""
try:
self.profiler.stop()
html_output = self.profiler.output_html()
async with aiofiles.open(self.output_path, 'w', encoding='utf-8') as f:
await f.write(html_output)
logger.info(f"Profiler stopped. Profile saved at: {self.output_path}")
except Exception as e:
logger.error(f"Failed to write profiler output to {self.output_path}: {e}", exc_info=True)🤖 Prompt for AI Agents
In agents-core/vision_agents/core/profiling/base.py around lines 19 to 23, the
async on_finish handler lacks a Google-style docstring, performs unguarded
synchronous file I/O, doesn't check profiler state, lacks encoding, and has no
error handling; update the method to include a Google-style docstring, verify
the profiler was started before calling stop(), use aiofiles for non-blocking
file writes with encoding='utf-8', wrap stop/output/write in a try/except and
log failures with context (including exception info), and log success once the
HTML is written.
Summary by CodeRabbit
New Features
Changes
Chores