[draft] Metamodel entity changes #49
Conversation
Signed-off-by: sachintendulkar576123 <[email protected]>
oi-raanne
requested review from
kshitiz-okahu,
anshul7665 and
oi-raanne
and removed request for
kshitiz-okahu and
anshul7665
| embed_model = self.instance.__dict__.get('_embed_model').__class__.__name__ | ||
|
|
||
| # Fallback to None or specific default values if not found | ||
| # temperature = self.instance.__dict__.get("temperature", None) |
There was a problem hiding this comment.
commented code can be removed if not needed, check other files as well
| def __init__(self, instance): | ||
| self.instance = instance | ||
|
|
||
|
|
There was a problem hiding this comment.
avoid consecutive new lines, single new line is generally enough
also have new line after all imports, and before beginning of classes and functions
please check other files as well for this consistency
|
|
||
| model_name = resolve_from_alias(self.instance.__dict__, ["model", "model_name"]) | ||
|
|
||
| span.set_attribute("entity.2.name", model_name) |
There was a problem hiding this comment.
could you check if its possible to avoid hardcoding the entity index, 2 in this case
| def __init__(self, instance): | ||
| self.instance = instance | ||
| def set_provider_name(self): | ||
| provider_url = "" |
There was a problem hiding this comment.
is it needed, its setting to empty string
| deployment_name = resolve_from_alias(self.instance.__dict__, ["engine", "azure_deployment", | ||
| "deployment_name", "deployment_id", "deployment"]) | ||
| span.set_attribute("entity.1.name","AzureOpenAI") | ||
| span.set_attribute("entity.1.type", "inference.azure_oai") |
There was a problem hiding this comment.
entity name and type should not be hardcoded but deduced in this case
at least for the type you can refer existing code, how its differentiating between openai vs az_oai vs sagemaker etc
@oi-raanne I think we still have to look at name vs provider_name in this case, we were discussing in previous PR but didn't reach conclusion yet
| # Check if _vector_store exists and get its class name | ||
|
|
||
| span.set_attribute("entity.1.name", provider) | ||
| span.set_attribute("entity.1.type", "vectorstore.chroma") |
There was a problem hiding this comment.
see if type can be deduced instead of hardcoded, what if another vectorstore is used instead
| # extract token uasge from langchain openai | ||
| if (response is not None and hasattr(response, "response_metadata")): | ||
| response_metadata = response.response_metadata | ||
| token_usage = response_metadata.get("token_usage") | ||
| d = {} |
| @@ -21,7 +25,7 @@ | |||
| PROVIDER = "provider_name" | |||
| EMBEDDING_MODEL = "embedding_model" | |||
| VECTOR_STORE = 'vector_store' | |||
|
|
|||
| META_DATA="meta_data" | |||
There was a problem hiding this comment.
metadata is single word, underscore is not needed
|
|
||
|
|
||
| def add_entities(self,span): | ||
| span.set_attribute("span_type","Inference") |
There was a problem hiding this comment.
all the 'keys' that we are adding are in lower case, refer https://github.com/monocle2ai/monocle/blob/main/src/monocle_apptrace/metamodel/entities/entity_types.json
so it would be inference instead of Inference
|
|
||
| # Check if model_name is valid and set the attributes accordingly | ||
| if embed_model: | ||
| span.set_attribute("entity.2.name", embedding_model) |
There was a problem hiding this comment.
can you accept the "2" string from constructor, hard coding it here could be problematic in future
| # extract token uasge from langchain openai | ||
| if (response is not None and hasattr(response, "response_metadata")): | ||
| response_metadata = response.response_metadata | ||
| token_usage = response_metadata.get("token_usage") | ||
| d = {} |
There was a problem hiding this comment.
Can you give this variable a self describing name
| pass | ||
| def add_provider_attributes(self, span): | ||
| # Assuming the provider details are in instance.provider | ||
| provider_name = "" |
There was a problem hiding this comment.
in python there is no need to initialize a variable twice.
Even if it declared inside the if statement, it can be used outside if statement as well.
| @@ -85,9 +89,15 @@ def pre_task_processing(to_wrap, instance, args, span): | |||
| update_span_with_infra_name(span, INFRA_SERVICE_KEY) | |||
|
|
|||
| #capture the tags attribute of the instance if present, else ignore | |||
| retreival = Retreival(instance) | |||
|
no longer needed as the changes have been incorporated in #51 |
Proposed changes
Added Attributes for Span Type Retrieval, Inference:
Updated wrap_common.py:
Types of changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyChecklist
Put an
xin 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...