-
Notifications
You must be signed in to change notification settings - Fork 167
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
GenAI events - yamlify body definitions and cosmetic examples improvements #1469
base: main
Are you sure you want to change the base?
Conversation
examples: ["get_weather"] | ||
requirement_level: required | ||
- id: arguments | ||
type: undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's clarify if these should be deserialized or provided as string.
The perf cost of deserialization compared to all other things necessary to create this event is small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A concern: In general the "arguments" string returned by (at least OpenAI -- my experience is limited) may not be valid JSON. In the (likely rare) case that it isn't this means that "arguments" would then, presumably, fallback to be a string. That could be a rare surprise (i.e. possible bug) to whatever system is processing this telemetry.
I suppose that is fine if the type allows both string and object (or perhaps AnyValue
would be used) -- then implementations need to cope.
Another possible concern: https://platform.openai.com/docs/api-reference/chat/create#chat-create-response_format includes the (surprising to me) warning:
Important: when using JSON mode, you must also instruct the model to produce JSON yourself via a system or user message. Without this, the model may generate an unending stream of whitespace until the generation reaches the token limit, resulting in a long-running and seemingly "stuck" request.
Not having the raw "arguments" string in the telemetry could make it hard to debug why a parsed JSON response of, say {"foo": "bar"}
, took 1024 (or however many) tokens.
What are reasonable use cases for having the parsed arguments in telemetry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed at GenAI call:
if max token is reached in the middle of the arguments, it's truncated, so let's keep it a string, but we might change it for some systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should also update the examples above
@@ -30,6 +70,73 @@ groups: | |||
brief: > | |||
This event describes the assistant message passed to GenAI system or received from it. | |||
extends: gen_ai.common.event.attributes | |||
body: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: add generated md table into the description
Merge requirement checklist
[chore]
schema-next.yaml updated with changes to existing conventions.