Skip to content

fix: resolve artifact refs in flattened multi-image items#51

Open
timzsu wants to merge 2 commits into
mainfrom
fix/vision-artifact-refs-after-flatten
Open

fix: resolve artifact refs in flattened multi-image items#51
timzsu wants to merge 2 commits into
mainfrom
fix/vision-artifact-refs-after-flatten

Conversation

@timzsu
Copy link
Copy Markdown
Collaborator

@timzsu timzsu commented May 16, 2026

Purpose

Fix multi-image vision inputs when grouped upstream artifact refs are flattened before image loading.

Changes

  • src/worker/executors/mixins/data.py — re-resolve flattened {path: ...} image artifact refs before fetching images.
  • tests/worker/test_data_mixin_lineage.py — add a regression test for grouped image artifact refs from an upstream expression.

Design

Keep the existing image fetch path unchanged. The fix only converts flattened artifact refs into the local path or URL shape that path already expects.

Test Plan

  • uv run pytest tests/worker/test_data_mixin_lineage.py
  • uv run ruff check tests/worker/test_data_mixin_lineage.py
  • uv run mypy tests/worker/test_data_mixin_lineage.py

Test Result

All targeted checks passed. Pre-commit hooks also passed when committing.


Pre-submission Checklist
  • I have read the contribution guidelines.
  • I have run pre-commit run --all-files and fixed any issues.
  • I have added or updated tests covering my changes (if applicable).
  • I have verified that uv run pytest tests/ passes locally.
  • If I changed shared schemas or proto definitions, I have checked downstream compatibility across Server and Worker.
  • If I changed the SDK or CLI, I have verified the affected packages work (uv sync --all-packages --group ci --frozen).
  • If this is a breaking change, I have prefixed the PR title with [BREAKING] and described migration steps above.
  • I have updated documentation or config examples if user-facing behavior changed.

…items

`_flatten_grouped_image_items` produces a flat list whose elements can
still be artifact refs (``{"path": ...}`` shapes) when the upstream
result expression returned grouped lists. The downstream image-fetch
loop expects either a PIL image or a URL/path string and raises when
it sees a dict, so multi-image rows fail with an opaque validation
error.

Hoist the upstream context/root-node bindings out of the ``expr``
branch and re-run ``maybe_resolve_artifact_ref`` over the flattened
items before the fetch loop. When ``items`` came from a literal list
the bindings stay ``None`` and ``maybe_resolve_artifact_ref`` is a
no-op, so non-vision callers are unaffected.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
@timzsu timzsu changed the title fix(worker): re-resolve artifact refs after flattening grouped image fix(worker): resolve artifact refs in flattened multi-image items May 16, 2026
@timzsu timzsu changed the title fix(worker): resolve artifact refs in flattened multi-image items fix: resolve artifact refs in flattened multi-image items May 16, 2026
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
@timzsu timzsu marked this pull request as ready for review May 16, 2026 13:45
@timzsu timzsu requested a review from kaiitunnz as a code owner May 16, 2026 13:45
Copy link
Copy Markdown
Collaborator

@kaiitunnz kaiitunnz left a comment

Choose a reason for hiding this comment

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

Overall looks good. I only have a small question.

if fetch_images:
items, image_group_sizes = self._flatten_grouped_image_items(items)
items = [
maybe_resolve_artifact_ref(item, context, root_node)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we call maybe_resolve_artifact_ref here, is the call on line 422 still needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants