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 CreateResult.request_id field for tracking model usage. #6058

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tongyu0924
Copy link
Contributor

@tongyu0924 tongyu0924 commented Mar 21, 2025

Why are these changes needed?

Related issue number

Closes #6049

Checks

@tongyu0924
Copy link
Contributor Author

@tongyu0924 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

agree

@ekzhu
Copy link
Collaborator

ekzhu commented Mar 21, 2025

Thanks for the PR, though I would like to hold on a bit before the questions are resolved: #6049 (comment)

Choose a reason for hiding this comment

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

A minor (semantic) technicality, but at least in both Open AI client and Anthropic, the id is generated by the LLM API and is not created at the time of the request. It is a response ID (which can be used to track what an LLM said in response to a request. One could theoretically track a request ID separately if needed.

@a-holm
Copy link

a-holm commented Mar 30, 2025

Thanks for adding the request_id field for better tracking! This is a useful addition.

Overall, the approach of adding the optional field to CreateResult and using safe extraction methods (getattr, .get) in the clients looks good.

However, there are two issues I found during review:

  1. Azure AI Client (create_stream): The logic to extract request_id in autogen_ext/models/azure/_azure_ai_client.py within the create_stream method appears incorrect. It attempts getattr(result, "request_id", None) where result is the CreateResult being constructed, not the Azure SDK response object. This needs to be revised to correctly fetch the ID from the appropriate SDK object (perhaps stored from an initial response or a chunk if available).
  2. Replay Client: The logic in autogen_ext/models/replay/_replay_chat_completion_client.py only extracts request_id if the replayed item is a dict. It should MAYBE if point 1 is on purpose also handle the case where the item is a CreateResult object, potentially using getattr(response, "request_id", None) in that scenario.

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.

Add CreateResult.request_id field for tracking model usage.
4 participants