-
Notifications
You must be signed in to change notification settings - Fork 139
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
Adding operation name to LLM spans #995
base: main
Are you sure you want to change the base?
Adding operation name to LLM spans #995
Conversation
Signed-off-by: sudivate <sudivate@microsoft.com>
Signed-off-by: sudivate <sudivate@microsoft.com>
… into sudivate/consistent-span-names Signed-off-by: sudivate <sudivate@microsoft.com>
Signed-off-by: sudivate <sudivate@microsoft.com>
model/registry/gen-ai.yaml
Outdated
stability: experimental | ||
type: string | ||
brief: The name of the LLM operation request being made. | ||
examples: ['chat.completions', 'embeddings', 'speech.generations', 'audio.transcriptions', 'audio.translations', 'image.generations'] |
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.
examples: ['chat.completions', 'embeddings', 'speech.generations', 'audio.transcriptions', 'audio.translations', 'image.generations'] | |
examples: ['chat', 'embeddings', 'speech.generations', 'audio.transcriptions', 'audio.translations', 'image.generations'] |
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.
I guess if we convert it into a enum, it would be both - chat
and completions
(legacy)?
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.
True, ideally it should be completions and chat.completions
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.
I find chat.completions
to be highly confusing with completions
. I'd stick with chat
and completions
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.
Left some minor comments. Please also add the changelog record.
Thank you!
model/registry/gen-ai.yaml
Outdated
stability: experimental | ||
type: string | ||
brief: The name of the LLM operation request being made. | ||
examples: ['chat.completions', 'embeddings', 'speech.generations', 'audio.transcriptions', 'audio.translations', 'image.generations'] |
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.
I guess if we convert it into a enum, it would be both - chat
and completions
(legacy)?
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.
The changes look good to me. Thanks
|
||
| Value | Description | Stability | | ||
| ---------------------- | -------------------- | ---------------------------------------------------------------- | | ||
| `completions` | Completions | ![Experimental](https://img.shields.io/badge/-experimental-blue) | |
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.
Howa about adding tokenize
as well?
tokenize(prompt, return_tokens=False)[source]
The text tokenize operation allows you to check the conversion of provided input to tokens for a given model. It splits text into words or sub-words, which then are converted to ids through a look-up table (vocabulary). Tokenization allows the model to have a reasonable vocabulary size.
We can potentially continue the discussion here after #955 is merged |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Fixes #
Changes
Adding operation name to LLM spans
Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.
Merge requirement checklist
[chore]