-
Notifications
You must be signed in to change notification settings - Fork 10
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
Initial metamodel definition #39
Conversation
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
"entity.2.name": "gpt-35-turbo", | ||
"entity.2.type": "Model.LLM", | ||
"entity.2.model_name": "gpt-35-turbo", | ||
"entity.2.temperature": 0.1 |
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.
temperature is per request and should be part of metadata
in events
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.
Ah ok. will fix it.
"span.type": "Inference", | ||
"entity.count": 2, | ||
"entity.1.name": "AzureOpenAI", | ||
"entity.1.type": "Inference.Azure_oai", |
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.
should we go all with small case for the value?
inference.azure_oai
"entity.count": 2, | ||
"entity.1.name": "AzureOpenAI", | ||
"entity.1.type": "Inference.Azure_oai", | ||
"entity.1.provider_name": "openai.azure.com", |
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.
what would be the difference between
entity.1.name
and entity.1.provider_name
- can we not merge them ?
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 believe provider name is an argument sent to Azure OpenAI inference API. Hence it's capturing here.
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.
provider_name is currently set to hostname of inference endpoint, so openai.azure.com
in case of azopenai
ref:
monocle/src/monocle_apptrace/wrap_common.py
Line 187 in cc825cc
def set_provider_name(curr_span, instance): |
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.
provider_name
is derived from the hostname of inference url for capturing it in span. It is not sent to openAI url.
# Monocle Entities | ||
The entity type defines the type of GenAI component that Monocle understand. The monocle instrumentation can extract the relevenat information for this entity. There are a fixed set of [entity types](./entity_types.py) that are defined by Monocle out of the box, eg workflow, model etc. As the GenAI landscape evolves, the Monocle community will introduce a new entity type if the current entities won't represent a new technology component. | ||
|
||
## Entity Types |
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.
can all the entity types be small case
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 would like to keep the vendor names starting with upper case. Do you have a strong preference all lowercase or if that's the convention generally followed, then we can switch to lower case.
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.
AWS
- capital case,
Azure
- pascal case,
NVIDIA
- capital case,
OpenAI
- pascal case,
HuggingFace
- pascal case
as type is in our control, we can keep it consistent.
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 .. I was trying to keep it consistent with the vendor name/brand, but I see your point. I guess it's better to be consistent here. Will change it all to all lowercase and camel case.
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
Signed-off-by: Prasad Mujumdar <[email protected]>
Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce to this project?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...