-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix the bug of lm in predict #774
base: main
Are you sure you want to change the base?
Conversation
Hi @minfawang , thanks for checking up on this. the lm is actually used as the line As for the Feel free to comment otherwise if you think this PR is still needed. Thanks! |
Thanks. The major glitch to fix is getting lm from the kwargs. See this part |
hmm I'm still a bit confused. Could you share an example perhaps that illustrates this issue? To my understanding, the current logic ensures the correct LM is loaded in, and we don't need to touch |
Below is an example: lm1 = dspy.OpenAI(model='gpt-3.5-turbo-0125', api_key=OPENAI_API_KEY)
lm2 = dspy.OpenAI(model='gpt-4-0125-preview', api_key=OPENAI_API_KEY)
dspy.settings.configure(lm=lm1) # <-- default to lm1
#Define a simple signature for basic question answering
class BasicQA(dspy.Signature):
"""Answer questions with short factoid answers."""
question = dspy.InputField()
answer = dspy.OutputField(desc="often between 1 and 5 words")
#Pass signature to ChainOfThought module
generate_answer = dspy.ChainOfThought(BasicQA)
# Call the predictor on a particular input.
question='What is the color of the sky?'
pred1 = generate_answer(question=question) # <-- Use the default lm1
pred2 = generate_answer(question=question, lm=lm2) # <-- lm2 temperature is used, but lm2 is ignored in generation The |
@minfawang This will actually work using the DSPy context manager. Feel free to reference this documentation on using multiple LMs together. If you run |
Thanks. I understand that I could use I think we should:
Are you more in favor of 2)? |
I think the change you've proposed (1) is making more sense to me now because we don't want to ignore the lm kwarg if passed in, but I'm realizing I don't believe this is needed but I'll confirm. If not, we can merge this PR with the changes as is. Thanks for sharing the examples - really helpful to dive deeper to isolate the use cases! |
Thanks for your patience! The plan sounds good. |
Please don't merge this yet. It's missing some context which I'll add asap |
In l71 of predict.py, there is a logic to use the lm from the kwargs:
lm = kwargs.pop("lm", self.lm) or dsp.settings.lm
, but thegenerate
call does not use it. This PR fixes the bug by using the lm as the context.Also, the previous comment stated that it's unclear why
query_only
should ever be used, and this is the only place thequery_only
option is used. So it sounds like a good choice to just get rid of this option and simplify the code logic.Reference: