Skip to content

Add Model Name and System Fingerprint to llm_output in _convert_response_to_chat_result #84

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

Merged
merged 7 commits into from
Mar 20, 2025

Conversation

jmoreno11
Copy link
Contributor

This PR enhances _convert_response_to_chat_result by including the model name and system fingerprint in llm_output. The model name is particularly important for observability tools like LangFuse, which rely on llm_output["model_name"] for tracking.

Currently, Databricks does not provide the model name in the metadata—only the endpoint name. This change ensures that external models are properly identified, improving observability and logging.

Changes:

  • Added model_name to llm_output, defaulting to self.model if not in the response.
  • Added system_fingerprint to llm_output for additional metadata.
  • Updated llm_output to include token_usage separately for better structure.

Why?
When using external models with Databricks, the returned metadata only includes the endpoint name, not the actual model name. This change allows users to extract the correct model name for logging and monitoring.

Testing:

  • Ensured the new metadata fields are correctly populated when present in the response.
  • Maintains backward compatibility by defaulting to self.model if response["model"] is missing.

usage = response.get("usage", {})
return ChatResult(generations=generations, llm_output=usage)
llm_output = {
"token_usage": response.get("usage", {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to usage so it doesn't change the API for existing users?

Suggested change
"token_usage": response.get("usage", {}),
"usage": response.get("usage", {}),

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 did as @bbqiu suggested, so usage remains intact

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 also created a new unit test, since the existing ones were not altered by this change

Copy link
Contributor

@sunishsheth2009 sunishsheth2009 left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

@bbqiu bbqiu left a comment

Choose a reason for hiding this comment

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

lgtm!

@jmoreno11
Copy link
Contributor Author

jmoreno11 commented Mar 19, 2025

Please reapprove, I fixed the linting error (ruff --fix)

P.S. I'm confused about this particular linting error: did you change the linting configuration recently? Otherwise it doesn't make sense to me

Jose Moreno Ortega and others added 2 commits March 19, 2025 21:58
@jmoreno11
Copy link
Contributor Author

Apologies, ruff cli was doing something weird. I created a PR on my forked branch and the linting is now fixed.
image

Copy link
Collaborator

@bbqiu bbqiu left a comment

Choose a reason for hiding this comment

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

approving to unblock merge

@bbqiu bbqiu merged commit 6018beb into databricks:main Mar 20, 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