-
Notifications
You must be signed in to change notification settings - Fork 423
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
feat(llmobs): submit langchain embedding spans #9850
Conversation
|
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.
Implementation looks great, just a couple questions 😄 can we add tests in test_langchain_llmobs.py
to class TestLLMObsLangchainCommunity
and class TestLangchainTraceStructureWithLlmIntegrations
?
I updated and addressed all the previous comments. The only thing left now is writing the test case but before doing that, I wanted to confirm the implementation details especially the parsing difference between the workflow vs non-workflow cases 🙏 |
BenchmarksBenchmark execution time: 2024-07-31 16:49:43 Comparing candidate commit 1601844 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 214 metrics, 2 unstable metrics. |
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.
Looks mostly great, I added a comment/suggestion on the input documents formatting.
Just needs tests, and release note similar to this PR.
Co-authored-by: Yun Kim <[email protected]>
6b6d966
to
edffa8b
Compare
fa1c650
to
8b22c4a
Compare
8b22c4a
to
af016f4
Compare
@yahya-mouman there are significant flakiness/skipping issues in the Langchain test suite. Let's merge this PR once #9768 is merged. |
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 be good to go after the testing comments are addressed now that the flakiness should be addressed!
78f08e0
to
c714c41
Compare
Great job on fixing the flakiness of the tests 🥇 I believe everything is done and good to go with this PR |
Changes
This PR adds instrumentation to submit langchain embedding spans to LLM Observability. The embedding spans sent to LLM Observability will contain the following I/O data:
input.documents: when part of a workflow captures the embeddings as a json list. when it is not a workflow but a single embedding operation captures each embedding input as a Document dictionary containing a text field
output.value: instead of unnecessarily storing very long vector values, adds a placeholder text [X embeddings returned with size Y]
Ticket : https://datadoghq.atlassian.net/browse/MLOB-937
Checklist
Reviewer Checklist