-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Support both dict and object for _process_response #8726
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
base: main
Are you sure you want to change the base?
Support both dict and object for _process_response #8726
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.
Pull Request Overview
This PR enables _process_response
to handle both object attributes and dictionary keys when processing OpenAI's native tools like web_search and interpreter. The change addresses a compatibility issue where litellm returns dictionaries for some tool responses instead of objects, causing DSPy to fail when accessing the type
property.
- Adds a helper function to safely access both object attributes and dictionary keys
- Updates response processing logic to work with both object and dictionary formats
- Maintains backward compatibility with existing object-based responses
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Thanks so much @kalanyuz !! Any chance some of the things in this PR should be (or even have been already) fixed upstream in LiteLLM instead? |
@okhat I believe this issue is unique to dspy side due to its tight coupling with LiteLLM for it's usage tracking implementation. We can contribute to LiteLLM by adding more transformations but it'd probably be come a cat-and-mouse game where dspy would break every time a model provider decides to introduce a new object / features in the 'usage' object. Would giving dspy the ability to fall back on basic dictionary make it more futureproof in this case? |
While I'd prefer to support this in litellm for the proper fix, I'm fine with the safely measure in dspy. Btw, should we convert the response into dict first so that we don't need to check the field and key every time? |
9f1a2d1
to
4cfb5ae
Compare
@TomeHirata is this better? |
return None | ||
if isinstance(obj, dict): | ||
return obj | ||
if hasattr(obj, "model_dump"): |
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.
can we do isinstance(obj, BaseModel)
instead?
if hasattr(obj, "model_dump"): | ||
return obj.model_dump() | ||
if hasattr(obj, "__dict__") and not isinstance(obj, dict): | ||
return {k: v for k, v in obj.__dict__.items() if not k.startswith("_")} |
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.
What is if not k.startswith("_")
for?
With @TomeHirata 's #8692 that adds support for ResponseAPI, we can already actually use OpenAI's native tools like web_search or interpreter by initiating dspy.LM as the following:
However, the current version of dspy would always fail, saying the response doesn't have the property
type
.Upon debugging further, I found out that this is because base_lm undergoes litellm's wrapper function that transform each item in output as object. However they haven't added support for web_search_call or interpreter yet, therefore returning a simple dictionary. This causes the dspy to fail despite having the capability to return results.
This pull request refactors the
_process_response
method indspy/clients/base_lm.py
to add dict support so that dspy does not have to wait for litellm to add support for those objects.