Fix/521 image collection placeholders#917
Conversation
Greptile SummaryThis PR updates image collection handling in structured prompt rendering. The main changes are:
Confidence Score: 3/5These issues should be fixed before merging.
Important Files Changed
|
|
Hi maintainers, this PR is ready for CI. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self._is_image_collection_value(field_value): | ||
| parts.append(rendered) | ||
| else: | ||
| parts.append(f"{field_name}: {rendered}") |
There was a problem hiding this comment.
For image-only collections this drops the outer field name entirely, and the dict branch below drops keys as well. A structure like photos_by_role={"before": img1, "after": img2} now renders only bare [Image N] tokens, so the prompt no longer tells the model which image is which. That loses semantic context for common image comparison and gallery prompts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pipelex/core/stuffs/structured_content.py
Line: 148-151
Comment:
**Image labels are dropped**
For image-only collections this drops the outer field name entirely, and the dict branch below drops keys as well. A structure like `photos_by_role={"before": img1, "after": img2}` now renders only bare `[Image N]` tokens, so the prompt no longer tells the model which image is which. That loses semantic context for common image comparison and gallery prompts.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| # Check if this container or its nested contents have images | ||
| # Check if it's a generic container type (list/tuple/dict). | ||
| # Example: list[ImageContent], tuple[ImageContent, ...], dict[str, ImageContent]. | ||
| if hasattr(field_specific_type, "__origin__") and field_specific_type.__origin__ in {list, tuple, dict}: # type: ignore[union-attr] |
There was a problem hiding this comment.
Now that dict is included here, check_generic_container_for_images iterates dict.__args__ indiscriminately, both the key type and value type. So a field typed dict[ImageContent, str] would be reported as
an image field via the key type, and downstream template analysis would treat it as an image reference.
In practice this is rare (Pipelex dicts are essentially always dict[str, ...]), but since we're adding the dict branch, let's make it robust. Could you tighten check_generic_container_for_images so that when
container_type.__origin__ is dict, it only recurses into the value type, not the key?
and add a test
| # Check if arg_type is itself a generic container - recurse! | ||
| if hasattr(arg_type, "__origin__") and arg_type.__origin__ in {list, tuple, dict}: # type: ignore[union-attr] | ||
| if check_generic_container_for_images(arg_type): |
There was a problem hiding this comment.
Top-level optionals are unwrapped before this helper runs, but optionals inside containers aren't. For list[ImageContent | None] or dict[str, ImageContent | None], the inner arg_type is a Union /
UnionType. Its __origin__ is typing.Union, which isn't in {list, tuple, dict}, and isinstance(arg_type, type) is False — so the loop falls through and the collection is missed. | with_images
validation would then reject a perfectly valid structure.
This is technically pre-existing (the same gap exists for list[ImageContent | None] before your change), so if you'd rather scope this PR tightly, we can land it separately. But it'd be a clean addition here:
when an arg_type is a Union/UnionType, iterate its non-None members and run the same checks on each.
|
Hey @matthewYang08 , thanks for your contribution and the fix, its great! but there is something missing: Issue #521 specifically names |
|
You can also merge dev inside your branch, it is behind |
2 participants
🔗 Related Issues
#521
📝 Description
🔄 Type of Change
🧪 Tests
📋 Checklist
Summary by cubic
Hide field names for image-only collections in prompt rendering to reduce noise, while keeping inner keys for
dictimage groups and labels for single-image fields. Extend nested image discovery to handledictcontainers. Addresses Linear 521.list,tuple,dict,ListContent); keepdictitem keys for context; keep field names when mixed with non-images.dict[...]as a generic container when searching for nested image fields.Written for commit d632696. Summary will update on new commits. Review in cubic