fix: handle video attachment for llm#7679
fix: handle video attachment for llm#7679xunxiing wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new Video handling logic for direct messages and quoted messages is nearly identical; consider extracting a small helper (e.g.,
append_video_attachment(req, video, is_reply=False)) to avoid duplication and keep future changes in one place. - For consistency with the existing file attachment text (
[File Attachment: name {file_name}, path {file_path}]), you may want to include the video name in the video attachment text, or otherwise make the formatting parallel so downstream consumers can parse them uniformly. - It may be worth adding basic error handling around
convert_to_file_path()(e.g., catching exceptions and skipping the attachment with a log) to prevent a single failing conversion from breaking the whole request construction.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Video handling logic for direct messages and quoted messages is nearly identical; consider extracting a small helper (e.g., `append_video_attachment(req, video, is_reply=False)`) to avoid duplication and keep future changes in one place.
- For consistency with the existing file attachment text (`[File Attachment: name {file_name}, path {file_path}]`), you may want to include the video name in the video attachment text, or otherwise make the formatting parallel so downstream consumers can parse them uniformly.
- It may be worth adding basic error handling around `convert_to_file_path()` (e.g., catching exceptions and skipping the attachment with a log) to prevent a single failing conversion from breaking the whole request construction.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces support for video attachments within the astr_main_agent.py file, allowing the agent to recognize and process video components in both direct and quoted messages. The review feedback recommends improving the implementation by including the video filename in the generated text description for better context and adding error handling to ensure that failures in video processing do not interrupt the entire request.
| elif isinstance(comp, Video): | ||
| video_path = await comp.convert_to_file_path() | ||
| req.extra_user_content_parts.append( | ||
| TextPart(text=f"[Video Attachment: path {video_path}]") | ||
| ) |
There was a problem hiding this comment.
建议在 TextPart 中包含文件名,这与 File 附件的处理方式保持一致,并能为 LLM 提供更多上下文信息。此外,由于 convert_to_file_path 涉及网络 I/O(下载),建议考虑添加异常处理,以防止单个视频处理失败导致整个请求中断。
| elif isinstance(comp, Video): | |
| video_path = await comp.convert_to_file_path() | |
| req.extra_user_content_parts.append( | |
| TextPart(text=f"[Video Attachment: path {video_path}]") | |
| ) | |
| elif isinstance(comp, Video): | |
| try: | |
| video_path = await comp.convert_to_file_path() | |
| video_name = os.path.basename(video_path) | |
| req.extra_user_content_parts.append( | |
| TextPart(text=f"[Video Attachment: name {video_name}, path {video_path}]") | |
| ) | |
| except Exception as exc: | |
| logger.error(f"Error processing video attachment: {exc}") |
| elif isinstance(reply_comp, Video): | ||
| video_path = await reply_comp.convert_to_file_path() | ||
| req.extra_user_content_parts.append( | ||
| TextPart( | ||
| text=f"[Video Attachment in quoted message: path {video_path}]" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
同样建议在引用消息的视频描述中包含文件名,以保持信息的一致性。
| elif isinstance(reply_comp, Video): | |
| video_path = await reply_comp.convert_to_file_path() | |
| req.extra_user_content_parts.append( | |
| TextPart( | |
| text=f"[Video Attachment in quoted message: path {video_path}]" | |
| ) | |
| ) | |
| elif isinstance(reply_comp, Video): | |
| try: | |
| video_path = await reply_comp.convert_to_file_path() | |
| video_name = os.path.basename(video_path) | |
| req.extra_user_content_parts.append( | |
| TextPart( | |
| text=f"[Video Attachment in quoted message: name {video_name}, path {video_path}]" | |
| ) | |
| ) | |
| except Exception as exc: | |
| logger.error(f"Error processing quoted video attachment: {exc}") |
Modifications / 改动点
修复 AstrBot 在处理用户上传的视频文件时,LLM 无法获取视频路径的问题。
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Handle video message attachments so LLM requests receive the corresponding file paths for both direct and quoted videos.
Bug Fixes: