Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 37 additions & 5 deletions praisonai_tools/video/motion_graphics/agent.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Motion graphics agent factory."""

import asyncio
import tempfile
from pathlib import Path
from typing import Union, Any
Expand Down Expand Up @@ -193,16 +194,47 @@ def create_motion_graphics_agent(
Workspace directory: {workspace}
"""

# Create tools.
# FileTools is a utility class with bound methods; pass the instance so the
# Agent can register read_file/write_file/list_files as callable tools.
# Create tools. Expose bound methods individually so the Agent (which
# treats each tool entry as a callable) can register them across all LLM
# adapters (OpenAI, Anthropic, LiteLLM). Passing class instances is not
# portable — the OpenAI adapter logs "Tool ... not recognized".
file_tools = FileTools()
render_tools = RenderTools(render_backend, workspace, max_retries)


# 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))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.


Comment on lines +204 to +210
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
def render_composition(
output_name: str = "video.mp4",
fps: int = 30,
quality: str = "standard",
) -> dict:
"""Render the motion graphics composition to MP4."""
result = asyncio.run(
render_tools.render_composition(
output_name=output_name, fps=fps, quality=quality
)
)
# Strip the raw bytes — they are not JSON-serializable for the next
# LLM turn and the file is already on disk at result["output_path"].
return {k: v for k, v in result.items() if k != "bytes"}

tool_callables = [
file_tools.read_file,
file_tools.write_file,
file_tools.list_files,
lint_composition,
render_composition,
]

# Create agent
agent = Agent(
instructions=base_instructions + "\n\n" + MOTION_GRAPHICS_SKILL,
tools=[file_tools, render_tools],
tools=tool_callables,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
tools=tool_callables,
tools=tool_callables + agent_kwargs.pop("tools", []),

llm=llm,
**agent_kwargs
)
Expand Down
10 changes: 8 additions & 2 deletions praisonai_tools/video/motion_graphics/backend_html.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,15 @@ async def _render_with_playwright(self, workspace: Path, opts: RenderOpts) -> Re
# Wait a bit for animations to settle
await page.wait_for_timeout(50)

# Capture frame
# Capture frame. Clip to the fixed 1920x1080 viewport —
# `full_page=True` can produce odd-height images when
# content overflows, which libx264 rejects (height must
# be divisible by 2).
frame_path = temp_path / f"frame_{frame:06d}.png"
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},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

)
Comment on lines +239 to +242
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
frame_paths.append(frame_path)

# Encode to MP4 using FFmpeg
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "praisonai-tools"
version = "0.2.25"
version = "0.2.26"
description = "Extended tools for PraisonAI Agents"
authors = [
{name = "Mervin Praison"}
Expand Down
43 changes: 29 additions & 14 deletions tests/unit/video/test_motion_graphics_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,24 @@ def __init__(self, instructions="", tools=None, llm="", **kwargs):


class MockFileTools:
"""Mock FileTools for testing."""

"""Mock FileTools for testing.

Exposes the same method surface as `praisonaiagents.tools.file_tools.FileTools`
so the factory can reference bound methods as tools.
"""

def __init__(self, base_dir=""):
self.base_dir = base_dir

def read_file(self, filepath, encoding="utf-8"):
return ""

def write_file(self, filepath, content, encoding="utf-8"):
return True

def list_files(self, directory=".", pattern="*"):
return []


class MockBackend:
"""Mock render backend for testing."""
Expand Down Expand Up @@ -115,7 +128,13 @@ def test_create_agent_defaults(self):

assert isinstance(agent, MockAgent)
assert agent.llm == "claude-sonnet-4"
assert len(agent.tools) == 2 # FileTools and RenderTools
# 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
Comment on lines +131 to +137
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
assert "motion graphics specialist" in agent.instructions.lower()

@patch('praisonai_tools.video.motion_graphics.agent.Agent', MockAgent)
Expand Down Expand Up @@ -207,14 +226,10 @@ def test_agent_tools_configuration(self):
with tempfile.TemporaryDirectory() as tmpdir:
agent = create_motion_graphics_agent(workspace=tmpdir)

assert len(agent.tools) == 2

# Check FileTools
file_tools = agent.tools[0]
assert isinstance(file_tools, MockFileTools)
assert file_tools.base_dir == str(tmpdir)

# Check RenderTools
render_tools = agent.tools[1]
assert isinstance(render_tools, RenderTools)
assert render_tools.workspace == Path(tmpdir)
# 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
Loading