fix(motion-graphics): correct FileTools import path + constructor (v0.2.25)#28
Conversation
Post-merge fixes for #27 caught via live end-to-end run of motion_graphics_team: - agent.py / team.py: import FileTools from praisonaiagents.tools.file_tools (not praisonaiagents.tools). FileTools is not exported via the tools package __getattr__, so 'from praisonaiagents.tools import FileTools' raises ImportError in production and causes motion_graphics_team() and create_motion_graphics_agent() to both fail with the generic 'praisonaiagents not available' ImportError. - agent.py: drop the base_dir kwarg when instantiating FileTools — the class takes no constructor args (methods are bound directly). - team.py: also catch AttributeError alongside ImportError so lazy attribute failures fall back cleanly to None stubs. - pyproject.toml: bump version 0.2.24 -> 0.2.25 Unit tests continue to pass because they mock out praisonaiagents. The bug only surfaces in live runs. Follow-up: add one non-mocked smoke test that actually instantiates motion_graphics_team().
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.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR updates FileTools imports and initialization in motion graphics modules, changing the import path from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the FileTools import path and instantiation across the video motion graphics modules, while also bumping the package version to 0.2.25. A potential issue was identified where removing the base_dir argument from FileTools might lead to file path inconsistencies if the agent does not use absolute paths within the designated 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. | ||
| file_tools = FileTools() |
There was a problem hiding this comment.
Since FileTools no longer accepts a base_dir in its constructor, it likely defaults to the current working directory for file operations. This poses a risk if the process CWD differs from the intended workspace (which is often a temporary directory), as the agent might write files where the RenderTools (which uses the workspace path) cannot find them. Adding an explicit instruction to the agent to use absolute paths within the workspace helps ensure consistency.
file_tools = FileTools()\n base_instructions += f'\\nCRITICAL: All file operations must use absolute paths within the workspace: {workspace}'There was a problem hiding this comment.
Pull request overview
Hotfix to address live-run failures when instantiating the motion graphics team/agent by correcting the FileTools import path and how it is constructed.
Changes:
- Update
FileToolsimport topraisonaiagents.tools.file_tools.FileToolsin motion graphics agent/team. - Change
FileToolsconstruction increate_motion_graphics_agent()to avoid passing unsupported constructor args. - Bump project version to
0.2.25.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyproject.toml |
Bumps package version for the hotfix release. |
praisonai_tools/video/motion_graphics/team.py |
Fixes FileTools import path; adjusts import error handling for search_web. |
praisonai_tools/video/motion_graphics/agent.py |
Fixes FileTools import path and updates instantiation to match upstream API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from praisonaiagents.tools.file_tools import FileTools | ||
| # search_web is lazily resolved via praisonaiagents.tools.__getattr__ | ||
| from praisonaiagents.tools import search_web | ||
| except (ImportError, AttributeError): | ||
| # Fallback for development / partial installs | ||
| Agent = None | ||
| AgentTeam = None | ||
| FileTools = None |
There was a problem hiding this comment.
FileTools is imported here but not used anywhere in this module. If it’s not needed for side effects, consider removing the import to reduce confusion and avoid unused-import lint failures in stricter environments.
| from praisonaiagents.tools.file_tools import FileTools | |
| # search_web is lazily resolved via praisonaiagents.tools.__getattr__ | |
| from praisonaiagents.tools import search_web | |
| except (ImportError, AttributeError): | |
| # Fallback for development / partial installs | |
| Agent = None | |
| AgentTeam = None | |
| FileTools = None | |
| # search_web is lazily resolved via praisonaiagents.tools.__getattr__ | |
| from praisonaiagents.tools import search_web | |
| except (ImportError, AttributeError): | |
| # Fallback for development / partial installs | |
| Agent = None | |
| AgentTeam = None |
| # 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. | ||
| file_tools = FileTools() |
There was a problem hiding this comment.
FileTools() is now instantiated without a workspace/base_dir, but the existing unit tests expect the FileTools instance to be scoped to the agent workspace (see tests/unit/video/test_motion_graphics_agent.py where file_tools.base_dir == str(tmpdir)). This change will break those tests and may also remove the intended sandboxing of file operations to the workspace. Consider configuring the FileTools instance to use workspace via the supported API (e.g., set a base_dir/root attribute after construction or wrap FileTools with a workspace-scoped adapter), and update the related tests accordingly.
| file_tools = FileTools() | |
| file_tools = FileTools() | |
| if hasattr(file_tools, "base_dir"): | |
| file_tools.base_dir = str(workspace) |
| try: | ||
| from praisonaiagents import Agent, AgentTeam | ||
| from praisonaiagents.tools import FileTools, search_web | ||
| except ImportError: | ||
| # Fallback for development | ||
| from praisonaiagents.tools.file_tools import FileTools | ||
| # search_web is lazily resolved via praisonaiagents.tools.__getattr__ | ||
| from praisonaiagents.tools import search_web | ||
| except (ImportError, AttributeError): | ||
| # Fallback for development / partial installs |
There was a problem hiding this comment.
Catching AttributeError across the entire import block can accidentally swallow real bugs raised during module import/initialization (not just a missing search_web lazy attribute), and then the code falls back to AgentTeam=None with a misleading "install" error. Consider narrowing the exception handling: keep the praisonaiagents import guarded by ImportError only, and handle search_web resolution in its own guarded block (or via getattr(..., None)) so genuine import-time attribute errors still surface.
Hotfix for live-run failures in
motion_graphics_team()/create_motion_graphics_agent()Caught via live run of
Which failed with:
…despite
praisonaiagentsbeing installed and importable. Unit tests never caught this because they mock outAgent/AgentTeam/FileTools.Root causes
Wrong import path for
FileToolspraisonai_tools/video/motion_graphics/agent.py:9andteam.py:9did:FileToolsis not exported by the tools package__getattr__(only lowercase keys likefile_tools,read_file, etc. are). The import raisesImportError, which the broadexcept ImportError:swallows — setting every symbol (Agent,AgentTeam,FileTools,search_web) toNoneand making the factory raise the misleading top-levelImportError.FileTools(base_dir=...)crashesagent.py:197didFileTools(base_dir=str(workspace)). The actual class takes no constructor args — methods are bound directly. Even if import had succeeded, the factory would still have raisedTypeError: FileTools() takes no arguments.Fix
Also catch
AttributeErroralongsideImportErrorinteam.pyso lazy-attribute failures fall back cleanly.Verification (live, not mocked)
Deterministic
HtmlRenderBackendpath (Example 01) still renders 1920×1080 MP4 end-to-end in ~5s. All 87/87 unit tests still green.Version
Bump
0.2.24→0.2.25(patch).Follow-up (not in this PR)
Add one non-mocked smoke test that actually instantiates
motion_graphics_team()andcreate_motion_graphics_agent()against the realpraisonaiagentsSDK, so future regressions of this class are caught by CI.Summary by CodeRabbit
Chores
Bug Fixes