fix(motion-graphics): agent tool wiring + ffmpeg frame clipping (v0.2.26)#29
Conversation
….26)
End-to-end agent loop now works with real LLMs (OpenAI gpt-4o-mini
verified). Caught by running the Example 03 agent factory live against
gpt-4o-mini and capturing the full failure chain. Unit tests previously
mocked these paths so the bugs were invisible.
Three real bugs fixed:
1. Tool registration (agent.py)
Passing class instances (FileTools(), RenderTools()) as tools caused the
OpenAI adapter to log 'Tool ... not recognized' and the agent to skip
tool calls entirely. Now we expose individual bound methods:
- file_tools.read_file / write_file / list_files
- lint_composition / render_composition (sync wrappers, see #2)
2. Async tools in sync agent path (agent.py)
RenderTools.lint_composition / render_composition are async but the
Agent sync path does not await coroutines. Result:
'Object of type coroutine is not JSON serializable'
Fix: wrap each async tool with a local sync function that uses
asyncio.run(...). Bytes are also stripped from the render_composition
return (unserializable, and the file already lives at output_path).
3. Odd-height screenshots break libx264 (backend_html.py)
page.screenshot(full_page=True) captured 1920x1167 for the LLM-authored
composition (SVG overflowed viewport). libx264 rejects odd height:
'height not divisible by 2 (1920x1167)'
Fix: clip to the exact 1920x1080 viewport via
page.screenshot(clip={x:0,y:0,width:1920,height:1080})
Tests updated (test_motion_graphics_agent.py):
- MockFileTools now exposes read_file/write_file/list_files methods
- tool-count assertions updated from 2 class instances to 5 callables
Verification (live, not mocked):
PRAISONAI_AUTO_APPROVE=true MOTION_LLM=gpt-4o-mini \
python examples/python/video/03_motion_graphics_agent_factory.py
Produces:
index.html (LLM-authored, 1031 bytes)
intro.mp4 (1920x1080 H.264 yuv420p, 30fps, 1.5s, 13.5KB)
87/87 unit tests pass. Version bump 0.2.25 -> 0.2.26.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the motion graphics agent to expose tools as individual callables, improving compatibility with various LLM adapters and resolving JSON serialization issues with async tools. It also addresses a video encoding failure by enforcing a fixed 1920x1080 viewport for screenshots to ensure dimensions are compatible with libx264. Feedback highlights potential RuntimeError risks when using asyncio.run in async environments, a potential TypeError due to duplicate tools arguments in the Agent constructor, and a suggestion to replace hardcoded dimensions with constants.
| # "Object of type coroutine is not JSON serializable". | ||
| def lint_composition(strict: bool = False) -> dict: | ||
| """Lint the motion graphics composition for common issues.""" | ||
| return asyncio.run(render_tools.lint_composition(strict=strict)) |
There was a problem hiding this comment.
Using asyncio.run inside a tool function can cause a RuntimeError if the agent is executed within an existing event loop (e.g., in a Jupyter notebook, a FastAPI application, or any async environment). While this bridges the sync tool path to the async backend, it is a known pitfall for library users. Consider if the Agent class can support async tools directly or if a more robust sync-to-async bridge (like checking for a running loop) is needed. This also applies to the render_composition wrapper.
| agent = Agent( | ||
| instructions=base_instructions + "\n\n" + MOTION_GRAPHICS_SKILL, | ||
| tools=[file_tools, render_tools], | ||
| tools=tool_callables, |
There was a problem hiding this comment.
If agent_kwargs contains a tools key, this call will raise a TypeError because tools is already passed as a keyword argument. It is safer to extract and merge any tools provided in agent_kwargs to allow users to extend the agent's capabilities.
| tools=tool_callables, | |
| tools=tool_callables + agent_kwargs.pop("tools", []), |
| await page.screenshot(path=str(frame_path), full_page=True) | ||
| await page.screenshot( | ||
| path=str(frame_path), | ||
| clip={"x": 0, "y": 0, "width": 1920, "height": 1080}, |
There was a problem hiding this comment.
The dimensions 1920x1080 are hardcoded here and in the viewport configuration. Consider defining these as constants (e.g., DEFAULT_WIDTH, DEFAULT_HEIGHT) at the module level to ensure consistency and make future adjustments easier, especially since the motion graphics skill guide also relies on these specific dimensions.
There was a problem hiding this comment.
Pull request overview
Hotfix to make the motion-graphics agent loop work end-to-end with real LLM adapters and to prevent FFmpeg/libx264 failures caused by odd-height Playwright screenshots; bumps package version to 0.2.26.
Changes:
- Expose tool callables as individual bound methods + sync wrappers in
create_motion_graphics_agent(instead of passing tool class instances). - Clip Playwright screenshots to a fixed 1920×1080 region to avoid odd-height frames that libx264 rejects.
- Update unit tests to reflect the new tool registration surface; bump project version.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
praisonai_tools/video/motion_graphics/agent.py |
Switch tools wiring to individual callables and add sync wrappers around async render/lint functions. |
praisonai_tools/video/motion_graphics/backend_html.py |
Force screenshot clipping to 1920×1080 to prevent odd-height PNGs and FFmpeg encode failures. |
tests/unit/video/test_motion_graphics_agent.py |
Adjust mocks and assertions for the new callable-based tool list. |
pyproject.toml |
Bump version 0.2.25 → 0.2.26. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Sync wrappers around the async render tools — the Agent's sync call path | ||
| # does not await coroutines automatically and would otherwise fail with | ||
| # "Object of type coroutine is not JSON serializable". | ||
| def lint_composition(strict: bool = False) -> dict: | ||
| """Lint the motion graphics composition for common issues.""" | ||
| return asyncio.run(render_tools.lint_composition(strict=strict)) | ||
|
|
There was a problem hiding this comment.
Using asyncio.run(...) inside these tool wrappers will raise RuntimeError: asyncio.run() cannot be called from a running event loop when the agent is invoked from an environment that already has an active loop (e.g., Jupyter, async web servers, or if the agent framework runs tool calls in async contexts). Consider adding a small helper that detects an existing running loop and, in that case, runs the coroutine in a dedicated thread / separate event loop (or exposes async tools and ensures the agent awaits them) so tool calls work reliably in both sync and async runtimes.
| await page.screenshot( | ||
| path=str(frame_path), | ||
| clip={"x": 0, "y": 0, "width": 1920, "height": 1080}, | ||
| ) |
There was a problem hiding this comment.
The new clip values duplicate the viewport size set earlier (1920x1080). To avoid future drift if the viewport changes, consider referencing a single source of truth (e.g., store viewport = {"width": 1920, "height": 1080} and derive the clip from it) rather than repeating magic numbers in multiple places.
| # Tools are now exposed as individual callables (bound methods + | ||
| # sync render wrappers): read_file, write_file, list_files, | ||
| # lint_composition, render_composition. | ||
| assert len(agent.tools) == 5 | ||
| tool_names = {getattr(t, "__name__", "") for t in agent.tools} | ||
| assert {"read_file", "write_file", "list_files", | ||
| "lint_composition", "render_composition"} <= tool_names |
There was a problem hiding this comment.
These assertions validate tool registration by name/count, but they don't exercise the newly introduced sync wrappers' behavior (the core hotfix): that lint_composition/render_composition are callable from sync code and that render_composition strips the non-JSON-serializable bytes field. Adding a focused unit test that invokes the two wrapper callables from agent.tools and asserts the returned dict is JSON-serializable (and lacks the bytes key) would better protect this fix from regressions.
Hotfix — agent loop works end-to-end with real LLMs (v0.2.26)
Live-tested Example 03 (
create_motion_graphics_agent) against gpt-4o-mini. It now produces a real MP4 from an LLM-authored HTML/GSAP composition.3 real bugs found by live testing (all invisible to mocked unit tests)
agent.pyFileTools()/RenderTools()class instances as tools → OpenAI adapter logsTool ... not recognizedand skips tool callsread_file, write_file, list_files, lint_composition, render_compositionagent.pylint_composition/render_compositionare async → sync path fails withObject of type coroutine is not JSON serializableasyncio.run(...); also strip unserialisablebytesfrom render returnbackend_html.pypage.screenshot(full_page=True)captured 1920×1167 when SVG overflowed → libx264:height not divisible by 2 (1920x1167)clip={x:0,y:0,width:1920,height:1080}Live verification
Produces:
ffprobe on
intro.mp4:Tests
87 / 87 unit tests pass (was 87/87). Updates to
test_motion_graphics_agent.py:MockFileToolsnow exposesread_file/write_file/list_filesmethods so the factory can reference them as bound methods2(class instances) to5(callables)Follow-ups that are not in this PR (keeps scope tight)
motion_graphics_team) still hits a separate SDK-level bug in hierarchical process ('Agent' object has no attribute 'execution') — needs a one-line fix inpraisonaiagents/agent/chat_mixin.py(self.execution→getattr(self, 'execution', None)). Filed separately.create_motion_graphics_agentagainst a cheap real model withPRAISONAI_AUTO_APPROVE=true.Version
0.2.25→0.2.26