-
Notifications
You must be signed in to change notification settings - Fork 0
GenAI Utils Inference #2
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
base: main
Are you sure you want to change the base?
Conversation
util/opentelemetry-util-genai/src/opentelemetry/util/genai/client.py
Outdated
Show resolved
Hide resolved
) -> LLMInvocation: | ||
with self._lock: | ||
invocation = self._llm_registry.pop(run_id) | ||
invocation.end_time = time.time() |
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.
this isn't thread safe all operations should be within the lock. The same for emitter.emit and same for emitter.init
util/opentelemetry-util-genai/src/opentelemetry/util/genai/client.py
Outdated
Show resolved
Hide resolved
util/opentelemetry-util-genai/src/opentelemetry/util/genai/emitters.py
Outdated
Show resolved
Hide resolved
start_time: float | ||
request_model: Optional[str] = None | ||
system: Optional[str] = None | ||
db_system: Optional[str] = None |
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's this used for?
|
||
def _message_to_event(message, system, framework) -> Optional[Event]: | ||
content = _get_property_value(message, "content") | ||
if content: |
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.
in the poc there was a todo item to check if content should be collected, is that no longer needed?
attributes = { | ||
# TODO: add below to opentelemetry.semconv._incubating.attributes.gen_ai_attributes | ||
"gen_ai.framework": framework, | ||
GenAI.GEN_AI_SYSTEM: system, |
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.
why is gen_ai.provider.name not included?
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 only see SYSTEM_NAME in our constants. I've added the provider_name manually, with a todo to update once the semconvs from python catches up
Description
Creation of tools for GenAI utils for generating spans for inference calls.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.