Skip to content

fix(portkey): do not drain generator content and capture image parts - #843

Open
SAMurai-16 wants to merge 15 commits into
open-telemetry:mainfrom
SAMurai-16:fix/portkey-generator-content-and-image-parts
Open

SAMurai-16 wants to merge 15 commits into
open-telemetry:mainfrom
SAMurai-16:fix/portkey-generator-content-and-image-parts

Conversation

@SAMurai-16

Copy link
Copy Markdown
Contributor

Description

To resolve this:

  • Only walk materialized Sequences (excluding str, bytes, and bytearray) in _prepare_input_messages so generator content is left unconsumed for the SDK.
  • Map image_url parts to UriPart (for remote URLs) and BlobPart (for inline base64 data: URLs) via image_from_url.
  • Support both dictionary parts and object instances with property access (SimpleNamespace/SDK models).
  • Add unit and integration tests covering generator content and image parts.

Fixes #829

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Added unit and integration tests in instrumentation/opentelemetry-instrumentation-genai-portkey/tests/test_chat_completions.py:

  • test_chat_completions_generator_content_not_consumed: verifies the caller's generator remains unconsumed while span attributes record parts: [].
  • test_chat_completions_image_parts: verifies remote URLs map to UriPart and inline data: URLs decode to BlobPart.
  • test_prepare_input_messages_*: unit tests covering generators, iterators, byte sequence handling, image URLs, and object models.

Executed test suite:

  • pytest instrumentation/opentelemetry-instrumentation-genai-portkey/tests -v (210 passed, 3 skipped)
  • Pre-commit lint and format checks (ruff, ruff-format passed)

Checklist

  • Followed the style guidelines of this project
  • Changelog updated if the change requires an entry (will add once PR number is assigned)
  • Unit tests added

Copilot AI balanced review requested due to automatic review settings October 3, 2026 13:07
@SAMurai-16
SAMurai-16 requested a review from a team as a code owner October 3, 2026 13:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Pull request dashboard status

Waiting on the author · refreshed 2026-11-01 08:52 UTC

Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):

  • Inline threads: 1
Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
  • Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

- Only walk materialized Sequences in _prepare_input_messages to leave generator content unconsumed for the SDK

- Map image_url parts to UriPart and BlobPart via image_from_url

- Add unit and integration tests covering generator content and image parts
@SAMurai-16
SAMurai-16 force-pushed the fix/portkey-generator-content-and-image-parts branch from 201b09b to a68881c Compare October 3, 2026 17:36
@opentelemetry-pr-dashboard

This comment has been minimized.

@SAMurai-16
SAMurai-16 requested a review from lmolkova October 16, 2026 18:55
@SAMurai-16

Copy link
Copy Markdown
Contributor Author

I bumped the opentelemetry-util-genai dependency floor to 1.3b0.dev and updated the test mocks so they properly handle the legacy HTTP _post interface in portkey-ai==1.0.0 alongside the modern openai_client wrapper, making sure both the oldest and latest CI test environments pass cleanly.

@SAMurai-16
SAMurai-16 requested a review from lmolkova October 21, 2026 11:09
@SAMurai-16
SAMurai-16 requested a balanced review from Copilot October 23, 2026 13:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@SAMurai-16
SAMurai-16 requested a review from lmolkova October 23, 2026 13:12

@sangkyoonnam sangkyoonnam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I checked 3348b084 and a local merge of it with main at a408e14e (which has #890), using an HTTP transport stub and portkey-ai 2.3.4. With content capture on, main sends "messages": [] when messages is a generator or a map(), including the async, stream=True and HTTP 400 cases. This PR sends both input messages in each case and records them in gen_ai.input.messages. The request now also carries every item from a nested content generator and from an assistant tool_calls generator. With capture off, main and this PR send the same request and record no message content.

Merging main at a408e14e conflicts in README.md and tests/requirements.oldest.txt. Main's util-genai is now 1.4b0.dev, so deps-check treats the 1.3b0.dev floor as an earlier release cycle and rejects the -e install of util-genai in the oldest requirements. On a408e14e, adding that line makes scripts/check_deps.py exit 1 with that error. The suggestion below removes it, and with it applied the file merges cleanly and keeps main's typing-extensions==4.7.1 pin. Apply it together with the main merge; the unmerged PR branch still needs the line. I haven't run the oldest env after this change.

Comment on lines +26 to +27

-e util/opentelemetry-util-genai

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this when merging main; deps-check on main rejects it.

Suggested change
-e util/opentelemetry-util-genai

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for testing this thoroughly!
I have merged upstream/main into this branch, removed -e util/opentelemetry-util-genai from tests/requirements.oldest.txt, and kept the typing-extensions==4.7.1 pin.

…or-content-and-image-parts

# Conflicts:
#	README.md
#	instrumentation/opentelemetry-instrumentation-genai-portkey/tests/requirements.oldest.txt
@SAMurai-16

Copy link
Copy Markdown
Contributor Author

@lmolkova i believe this PR is ready for review.

@@ -0,0 +1 @@
Do not consume generator message ``content`` before the SDK call, and capture ``image_url`` parts as image parts.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR now also stops draining messages/tools/tool_calls generators and records audio, file and refusal parts - could the entry mention those?

Suggested change
Do not consume generator message ``content`` before the SDK call, and capture ``image_url`` parts as image parts.
Do not consume generator ``messages``, ``tools``, ``tool_calls`` or message ``content`` before the SDK call, and capture ``image_url``, ``input_audio``, ``file`` and ``refusal`` content parts.

The PR description could use the same update - it still describes walking only Sequences and lists test_prepare_input_messages_* tests that are no longer there.

Comment on lines +746 to +756
if hasattr(p.chat.completions, "openai_client"):
call_args = p.chat.completions.openai_client.with_raw_response.chat.completions.create.call_args
received_messages = call_args.kwargs["messages"]
else:
call_args = p.chat.completions._post.call_args
body = (
call_args.kwargs.get("body")
if isinstance(call_args.kwargs.get("body"), dict)
else call_args.kwargs
)
received_messages = body["messages"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: this "what did the SDK get" block is repeated in several new tests. A small helper would shorten them:

def _sent_messages(client):
    completions = client.chat.completions
    if hasattr(completions, "openai_client"):
        create = completions.openai_client.with_raw_response.chat.completions.create
        return create.call_args.kwargs["messages"]
    call_args = completions._post.call_args
    body = call_args.kwargs.get("body")
    return (body if isinstance(body, dict) else call_args.kwargs)["messages"]

received_messages = body["messages"]
received_content = received_messages[0]["content"]
assert not isinstance(received_content, list)
assert isinstance(received_content, Iterator)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A used-up generator is still an Iterator, so this check (and the same one in generator_messages_and_tools_captured and nested_generators_captured) passes on main too, where the content is drained before the SDK call. Could _consume_chat_request keep what it read, so the tests check the SDK got the full content?

_sent: list[Any] = []

def _consume_chat_request(*args: Any, **kwargs: Any) -> None:
    ...
    _sent.append([{**m, "content": list(m["content"])} for m in messages])
assert _sent[-1][0]["content"] == [
    {"type": "text", "text": "item 0"},
    {"type": "text", "text": "item 1"},
]

@SAMurai-16
SAMurai-16 requested a review from lmolkova November 1, 2026 09:52

This branch has not been deployed

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

[genai-portkey] With content capture enabled, generator/iterator message content is consumed and sent empty

4 participants