openai: apply instrumentation to beta.chat.Completions.parse #65
Conversation
| ) | ||
|
|
||
| # second call events | ||
| tool_call_telemetry = previous_message["tool_calls"] |
There was a problem hiding this comment.
This is the only diff from test_chat. Though there it's also sort of a coincidence that semconv matches the API response, while in this case it doesn't since there's additional fields.
|
|
||
| def _patch(self, _module): | ||
| def _patch(self, module): | ||
| version = tuple([int(x) for x in getattr(getattr(module, "version"), "VERSION").split(".")]) |
There was a problem hiding this comment.
Let me know if this is the sensible pattern for patching newer modules
There was a problem hiding this comment.
You can use module.version.VERSION directly since you are not adding any default to getattr though.
Problem with this approach is when these would be promoted out of beta, I expect the version check will pass but the patching will fail. I think a more robust approach would be to try to wrap anyway and catch the ModuleNotFoundError and AttributeError exceptions and set the beta_chat_available boolean accordingly.
codefromthecrypt
left a comment
codefromthecrypt
left a comment
There was a problem hiding this comment.
Wow great to see this coverage with little code.
I suspect a changelog is warranted.
With this change alone, does DeepEval work?
|
Yup verified DeepEval traces well with it |
|
I can verify tracing works with deepeval. If I switch to |
|
merging to make my workshop today less embarrassing as pip from main is better than someone's branch. we should get a patch released soon @xrmx ideally with the warning from #65 (comment) squashed, too |
3 participants
What does this pull request do?
Applies the openai chat instrumentation as-is to
beta.chat.Completions.parse-parsehas some additional fields to handle structured outputs but otherwise has the same interface as the normalchat.Completions.create, except it lacksstream=True. So we can get instrumentation of non-streaming by just applying the same instrumentation - because semconv doesn't define anything for structured output, I guess there isn't anything else to do.Streaming will require instrumenting the
streammethod which is a completely different interface than normal streaming but can be added later.The test cases overlap with
chatso no new casetteRelated issues
#64 - instruments non-streaming only