修复图片转述模型只能单线程运行的问题。现在他可以多线程转述 - #9748
Conversation
Refactor message handling to check for image or plain components instead of context content. Update group chat context handling to avoid duplicate image captioning.
Refactor handle_message to support optional image captioning and add background processing for image captions.
Add JSON handling for card data in group chat context.
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/builtin_stars/astrbot/group_chat_context.py" line_range="259-266" />
<code_context>
+ prompt: str,
+ ) -> None:
+ """Resolve image captions in the background and update one record."""
+ results = await asyncio.gather(
+ *(
+ self.get_image_caption(image_url, provider_id, prompt)
+ for _, image_url in pending_images
+ ),
+ return_exceptions=True,
+ )
+
+ resolved_message = caption_template
+ for (marker, _), result in zip(pending_images, results, strict=True):
+ if isinstance(result, BaseException):
+ logger.error("Failed to get image caption: %s", result)
+ replacement = " [Image]"
+ else:
</code_context>
<issue_to_address>
**suggestion:** Improve logging for per-image caption failures to include traceback and image context.
Currently only the exception message is logged, without a traceback or identifying which image failed, which limits diagnosability.
Consider:
- Using `logger.exception("Failed to get image caption for %s", url)` (or `exc_info=True`) so the traceback is captured.
- Including an image identifier (e.g., URL or hash) in the log message so specific failures can be correlated.
This preserves the non-blocking behavior while making failures easier to debug.
```suggestion
resolved_message = caption_template
for (marker, image_url), result in zip(pending_images, results, strict=True):
if isinstance(result, BaseException):
logger.error(
"Failed to get image caption for %s (marker %s)",
image_url,
marker,
exc_info=result,
)
replacement = " [Image]"
else:
replacement = f" [Image: {result}]"
resolved_message = resolved_message.replace(marker, replacement, 1)
```
</issue_to_address>
### Comment 2
<location path="astrbot/builtin_stars/astrbot/group_chat_context.py" line_range="278" />
<code_context>
+
+ logger.debug(f"group_chat_context captioned | {umo} | {resolved_message}")
+
+ def _format_message(
+ self,
+ event: AstrMessageEvent,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring image caption handling to use structured image slot metadata and a helper for task orchestration instead of template markers and parallel lists.
You can keep the new feature but reduce complexity by separating concerns and avoiding the parallel `template_parts` + marker juggling.
### 1. Narrow `_format_message` and avoid template markers
Instead of building both `parts` and `template_parts` and synthetic marker strings, return a single string plus structured image slot metadata that can be used by the background task.
For example:
```python
@dataclass
class ImageSlot:
index: int # position in parts list
url: str
def _format_message(
self,
event: AstrMessageEvent,
cfg: dict,
*,
caption_images: bool,
) -> tuple[str, list[ImageSlot]]:
datetime_str = datetime.datetime.now().strftime("%H:%M:%S")
prefix = f"[{event.message_obj.sender.nickname}/{datetime_str}]: "
parts: list[str] = [prefix]
image_slots: list[ImageSlot] = []
for comp in event.get_messages():
if isinstance(comp, Plain):
parts.append(f" {comp.text}")
elif isinstance(comp, Image):
url = comp.url if comp.url else comp.file
parts.append(" [Image]")
if caption_images and cfg["image_caption"]:
if url:
image_slots.append(ImageSlot(index=len(parts) - 1, url=url))
else:
logger.error("Failed to get image caption: image URL is empty.")
elif isinstance(comp, Json):
# unchanged JSON handling...
...
elif isinstance(comp, At):
is_at_self = str(comp.qq) in (event.get_self_id(), "all")
if is_at_self:
parts.insert(1, "⚠️[DIRECTED AT YOU] ")
parts.append(f" [At: {comp.name}]")
elif isinstance(comp, Reply):
# unchanged reply handling...
...
return "".join(parts), image_slots
```
Then `_fill_image_captions` can work directly on the stored record using indices instead of markers:
```python
async def _fill_image_captions(
self,
*,
umo: str,
record_id: str,
image_slots: list[ImageSlot],
provider_id: str,
prompt: str,
) -> None:
results = await asyncio.gather(
*(self.get_image_caption(slot.url, provider_id, prompt) for slot in image_slots),
return_exceptions=True,
)
async with self._get_lock(umo):
record_ids = self._record_ids.get(umo)
records = self.raw_records.get(umo)
if not record_ids or not records or record_id not in record_ids:
return
record_index = record_ids.index(record_id)
original = records[record_index]
parts = list(original) # or re-split if needed; e.g. store `parts` instead of joined string
for slot, result in zip(image_slots, results, strict=True):
if isinstance(result, BaseException):
logger.error("Failed to get image caption: %s", result)
caption_text = " [Image]"
else:
caption_text = f" [Image: {result}]"
parts[slot.index] = caption_text
resolved_message = "".join(parts)
records[record_index] = resolved_message
logger.debug(f"group_chat_context captioned | {umo} | {resolved_message}")
```
This removes:
- Synthetic marker strings and `.replace(..., 1)` calls.
- The duplicated `parts`/`template_parts` maintenance.
- The need to encode captioning state inside a secondary template representation.
### 2. Extract caption task orchestration out of `handle_message`
You can keep `handle_message` closer to its original “store record” responsibility by moving the task scheduling into a helper:
```python
async def handle_message(
self,
event: AstrMessageEvent,
*,
caption_images: bool = True,
) -> None:
if event.get_message_type() != MessageType.GROUP_MESSAGE:
return
umo = event.unified_msg_origin
cfg = self.cfg(event)
final_message, image_slots = self._format_message(
event,
cfg,
caption_images=caption_images,
)
record_id = uuid.uuid4().hex
async with self._get_lock(umo):
records = self.raw_records[umo]
record_ids = self._record_ids[umo]
records.append(final_message)
record_ids.append(record_id)
_trim_left(records, cfg["group_message_max_cnt"], record_ids)
event.set_extra("_group_context_record_id", record_id)
event.set_extra("_group_context_raw_idx", len(records) - 1)
self._maybe_schedule_caption_task(
umo=umo,
record_id=record_id,
image_slots=image_slots,
cfg=cfg,
)
logger.debug(f"group_chat_context | {umo} | {final_message}")
def _maybe_schedule_caption_task(
self,
*,
umo: str,
record_id: str,
image_slots: list[ImageSlot],
cfg: dict,
) -> None:
if not image_slots:
return
task = asyncio.create_task(
self._fill_image_captions(
umo=umo,
record_id=record_id,
image_slots=image_slots,
provider_id=cfg["image_caption_provider_id"],
prompt=cfg["image_caption_prompt"],
)
)
self._caption_tasks.add(task)
task.add_done_callback(self._on_caption_task_done)
```
This keeps the new functionality but:
- Restores `_format_message` to a single primary representation (string + structured slots).
- Moves caption task setup into a focused helper, making `handle_message` easier to read.
- Eliminates parallel `template_parts` and brittle marker replacement.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| resolved_message = caption_template | ||
| for (marker, _), result in zip(pending_images, results, strict=True): | ||
| if isinstance(result, BaseException): | ||
| logger.error("Failed to get image caption: %s", result) | ||
| replacement = " [Image]" | ||
| else: | ||
| replacement = f" [Image: {result}]" | ||
| resolved_message = resolved_message.replace(marker, replacement, 1) |
There was a problem hiding this comment.
suggestion: Improve logging for per-image caption failures to include traceback and image context.
Currently only the exception message is logged, without a traceback or identifying which image failed, which limits diagnosability.
Consider:
- Using
logger.exception("Failed to get image caption for %s", url)(orexc_info=True) so the traceback is captured. - Including an image identifier (e.g., URL or hash) in the log message so specific failures can be correlated.
This preserves the non-blocking behavior while making failures easier to debug.
| resolved_message = caption_template | |
| for (marker, _), result in zip(pending_images, results, strict=True): | |
| if isinstance(result, BaseException): | |
| logger.error("Failed to get image caption: %s", result) | |
| replacement = " [Image]" | |
| else: | |
| replacement = f" [Image: {result}]" | |
| resolved_message = resolved_message.replace(marker, replacement, 1) | |
| resolved_message = caption_template | |
| for (marker, image_url), result in zip(pending_images, results, strict=True): | |
| if isinstance(result, BaseException): | |
| logger.error( | |
| "Failed to get image caption for %s (marker %s)", | |
| image_url, | |
| marker, | |
| exc_info=result, | |
| ) | |
| replacement = " [Image]" | |
| else: | |
| replacement = f" [Image: {result}]" | |
| resolved_message = resolved_message.replace(marker, replacement, 1) |
|
|
||
| logger.debug(f"group_chat_context captioned | {umo} | {resolved_message}") | ||
|
|
||
| def _format_message( |
There was a problem hiding this comment.
issue (complexity): Consider refactoring image caption handling to use structured image slot metadata and a helper for task orchestration instead of template markers and parallel lists.
You can keep the new feature but reduce complexity by separating concerns and avoiding the parallel template_parts + marker juggling.
1. Narrow _format_message and avoid template markers
Instead of building both parts and template_parts and synthetic marker strings, return a single string plus structured image slot metadata that can be used by the background task.
For example:
@dataclass
class ImageSlot:
index: int # position in parts list
url: str
def _format_message(
self,
event: AstrMessageEvent,
cfg: dict,
*,
caption_images: bool,
) -> tuple[str, list[ImageSlot]]:
datetime_str = datetime.datetime.now().strftime("%H:%M:%S")
prefix = f"[{event.message_obj.sender.nickname}/{datetime_str}]: "
parts: list[str] = [prefix]
image_slots: list[ImageSlot] = []
for comp in event.get_messages():
if isinstance(comp, Plain):
parts.append(f" {comp.text}")
elif isinstance(comp, Image):
url = comp.url if comp.url else comp.file
parts.append(" [Image]")
if caption_images and cfg["image_caption"]:
if url:
image_slots.append(ImageSlot(index=len(parts) - 1, url=url))
else:
logger.error("Failed to get image caption: image URL is empty.")
elif isinstance(comp, Json):
# unchanged JSON handling...
...
elif isinstance(comp, At):
is_at_self = str(comp.qq) in (event.get_self_id(), "all")
if is_at_self:
parts.insert(1, "⚠️[DIRECTED AT YOU] ")
parts.append(f" [At: {comp.name}]")
elif isinstance(comp, Reply):
# unchanged reply handling...
...
return "".join(parts), image_slotsThen _fill_image_captions can work directly on the stored record using indices instead of markers:
async def _fill_image_captions(
self,
*,
umo: str,
record_id: str,
image_slots: list[ImageSlot],
provider_id: str,
prompt: str,
) -> None:
results = await asyncio.gather(
*(self.get_image_caption(slot.url, provider_id, prompt) for slot in image_slots),
return_exceptions=True,
)
async with self._get_lock(umo):
record_ids = self._record_ids.get(umo)
records = self.raw_records.get(umo)
if not record_ids or not records or record_id not in record_ids:
return
record_index = record_ids.index(record_id)
original = records[record_index]
parts = list(original) # or re-split if needed; e.g. store `parts` instead of joined string
for slot, result in zip(image_slots, results, strict=True):
if isinstance(result, BaseException):
logger.error("Failed to get image caption: %s", result)
caption_text = " [Image]"
else:
caption_text = f" [Image: {result}]"
parts[slot.index] = caption_text
resolved_message = "".join(parts)
records[record_index] = resolved_message
logger.debug(f"group_chat_context captioned | {umo} | {resolved_message}")This removes:
- Synthetic marker strings and
.replace(..., 1)calls. - The duplicated
parts/template_partsmaintenance. - The need to encode captioning state inside a secondary template representation.
2. Extract caption task orchestration out of handle_message
You can keep handle_message closer to its original “store record” responsibility by moving the task scheduling into a helper:
async def handle_message(
self,
event: AstrMessageEvent,
*,
caption_images: bool = True,
) -> None:
if event.get_message_type() != MessageType.GROUP_MESSAGE:
return
umo = event.unified_msg_origin
cfg = self.cfg(event)
final_message, image_slots = self._format_message(
event,
cfg,
caption_images=caption_images,
)
record_id = uuid.uuid4().hex
async with self._get_lock(umo):
records = self.raw_records[umo]
record_ids = self._record_ids[umo]
records.append(final_message)
record_ids.append(record_id)
_trim_left(records, cfg["group_message_max_cnt"], record_ids)
event.set_extra("_group_context_record_id", record_id)
event.set_extra("_group_context_raw_idx", len(records) - 1)
self._maybe_schedule_caption_task(
umo=umo,
record_id=record_id,
image_slots=image_slots,
cfg=cfg,
)
logger.debug(f"group_chat_context | {umo} | {final_message}")
def _maybe_schedule_caption_task(
self,
*,
umo: str,
record_id: str,
image_slots: list[ImageSlot],
cfg: dict,
) -> None:
if not image_slots:
return
task = asyncio.create_task(
self._fill_image_captions(
umo=umo,
record_id=record_id,
image_slots=image_slots,
provider_id=cfg["image_caption_provider_id"],
prompt=cfg["image_caption_prompt"],
)
)
self._caption_tasks.add(task)
task.add_done_callback(self._on_caption_task_done)This keeps the new functionality but:
- Restores
_format_messageto a single primary representation (string + structured slots). - Moves caption task setup into a focused helper, making
handle_messageeasier to read. - Eliminates parallel
template_partsand brittle marker replacement.
Updated the GroupChatContext tests to include caption_images handling for messages with images. Added tests for image captioning behavior in group messages.
Modifications / 改动点
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
Allow group image captioning to run concurrently in the background without duplicating captions for messages handled by the LLM pipeline.
Bug Fixes:
Enhancements:
Tests: