Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(openai): streaming tool_call + logging multiple tool_call #463

Merged
merged 8 commits into from
Oct 16, 2024

Conversation

guillaumefontan
Copy link
Contributor

@guillaumefontan guillaumefontan commented Oct 8, 2024

Fixes #464 and #465

@CLAassistant
Copy link

CLAassistant commented Oct 8, 2024

CLA assistant check
All committers have signed the CLA.

@guillaumefontan guillaumefontan changed the title Bugfix - openai streaming tool_call + logging multiple tool_call fix/openai streaming tool_call + logging multiple tool_call Oct 8, 2024
@guillaumefontan guillaumefontan changed the title fix/openai streaming tool_call + logging multiple tool_call fix: openai streaming tool_call + logging multiple tool_call Oct 8, 2024
@nirga nirga changed the title fix: openai streaming tool_call + logging multiple tool_call fix(openai): streaming tool_call + logging multiple tool_call Oct 8, 2024
Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

Thanks! Left a small nit comment

}
}
}
// if (chunk.choices[0]?.delta.tool_calls) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All done thanks!

@guillaumefontan
Copy link
Contributor Author

guillaumefontan commented Oct 8, 2024

@nirga just a note, this will break anyone's code which uses
span.attributes['${SpanAttributes.LLM_COMPLETIONS}.0.function_call.name'] (will return undefined)
as the new format will become
span.attributes['${SpanAttributes.LLM_COMPLETIONS}.0.function_call.0.name'].
To preserve backward compatibility, we could make it so that the first tool call remains
span.attributes['${SpanAttributes.LLM_COMPLETIONS}.0.function_call.name']
while all other tool calls are indexed:
span.attributes['${SpanAttributes.LLM_COMPLETIONS}.0.function_call.1.name'].
However this may be confusing.

@nirga
Copy link
Member

nirga commented Oct 9, 2024

@guillaumefontan I think we should match the way we've implemented this on Python -
https://github.com/traceloop/openllmetry/blob/a1b82d8b272ce3861d88f5a2898d3170aa9a6df0/packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py#L448

So it should be tool_calls (plural)

Also note the CI is broken on your PR

@guillaumefontan
Copy link
Contributor Author

Hi @nirga I've updated to tool_calls and fixed the lint warnings. However the build error:
Run pip3 install chromadb error: externally-managed-environment
does not appear to be related to my changes. Let me know if there is anything I can do.

Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here, working on fixing the CI now. LGTM except for that!

@nirga nirga merged commit 5d5de09 into traceloop:main Oct 16, 2024
4 checks passed
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.

Tool_call span attributes do not get added for openai stream completions
3 participants