Skip to content
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

Add MLflow tracing for langchain and openai implementations of VectorSearchRetrieverTool #43

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

leonbi100
Copy link
Contributor

@leonbi100 leonbi100 commented Jan 14, 2025

This PR adds tracing to the VectorSearchRetrieverTool across the Langchain and Openai implementations. In order to name the trace correctly I've implemented a reusable decorator factory:

def vector_search_retriever_tool_trace(func):
    """
    Decorator factory to trace VectorSearchRetrieverTool with the tool name
    """

Our tools are named based on the user input tool_name otherwise it defaults to the index name.

Tested in UI and it displays the retriever trace correctly:
image

Copy link
Contributor

@annzhang-db annzhang-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 😄 - could we also include a manual test so that we can confirm that the UI displays the retriever trace correctly?

vector_search_tool._run("Databricks Agent Framework")
trace = mlflow.get_last_active_trace()
spans = trace.search_spans(name=tool_name or index_name, span_type=SpanType.RETRIEVER)
assert len(spans) == 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we validate that the span logs the correct/expected input and output?

)
trace = mlflow.get_last_active_trace()
spans = trace.search_spans(name=tool_name or index_name, span_type=SpanType.RETRIEVER)
assert len(spans) == 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also validate input/output here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this as is for now. Discussed offline, but as it stands the current span does not have the correct input/output until this PR merges. I will wait to update the unit test.

@leonbi100 leonbi100 requested a review from annzhang-db January 16, 2025 02:34
@leonbi-db leonbi-db merged commit ceec040 into databricks:main Jan 16, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants