Skip to content

Conversation

@drawal1
Copy link
Contributor

@drawal1 drawal1 commented Apr 9, 2024

Supported model families: Mistral, Anthropic, Meta

This replaced PR #772 which is closed. Code has been updated to satisfy review comments on PR #772 posted by @arnavsinghvi11

@drawal1 drawal1 marked this pull request as ready for review April 9, 2024 19:40
@drawal1
Copy link
Contributor Author

drawal1 commented Apr 9, 2024

@arnavsinghvi11 - Thanks for the review! I have updated all the code per your review comments.

Notes

  1. I have deleted the old bedrock.py since it is obsolete and the name would conflict with the new aws provider class called Bedrock
  2. There is a test_aws_models.py file under /tests/modules which does not have test fixtures for aws credentials etc. so its not ready for pytest yet, but it is a nice, comprehensive test for all the aws models and providers and also illustrates a helper function that makes it easy to use aws providers and models

@sachinkun21
Copy link

ChainOfThought.forward is throwing error due to num_generations being removed in AWSAnthropic at following line:
---> 70 if (temperature is None or temperature <= 0.15) and num_generations > 1:

Here's the full stack trace

TypeError Traceback (most recent call last)
Cell In[63], line 3
1 # Define a module (ChainOfThought) and assign it a signature (return an answer, given a question).
2 qa = dspy.ChainOfThought('question -> answer')
----> 3 response = qa(question="Who is MS Dhoni?")
4 print(response.answer)

File /opt/conda/lib/python3.10/site-packages/dspy/predict/predict.py:49, in Predict.call(self, **kwargs)
48 def call(self, **kwargs):
---> 49 return self.forward(**kwargs)

File /opt/conda/lib/python3.10/site-packages/dspy/predict/chain_of_thought.py:59, in ChainOfThought.forward(self, **kwargs)
57 signature = new_signature
58 # template = dsp.Template(self.signature.instructions, **new_signature)
---> 59 return super().forward(signature=signature, **kwargs)

File /opt/conda/lib/python3.10/site-packages/dspy/predict/predict.py:70, in Predict.forward(self, **kwargs)
67 if num_generations is None:
68 num_generations = lm.kwargs.get("n", lm.kwargs.get("num_generations", None))
---> 70 if (temperature is None or temperature <= 0.15) and num_generations > 1:
71 config["temperature"] = 0.7
72 # print(f"#> Setting temperature to 0.7 since n={num_generations} and prior temperature={temperature}.")
73
74 # All of the other kwargs are presumed to fit a prefix of the signature.
75 # That is, they are input variables for the bottom most generation, so
76 # we place them inside the input - x - together with the demos.

TypeError: '>' not supported between instances of 'NoneType' and 'int'

@drawal1
Copy link
Contributor Author

drawal1 commented Apr 10, 2024

@sachinkun21 - can you get the latest code for the PR and doublecheck that aws_models.py contains below code?

        # Anthropic models do not support the following parameters
        query_args.pop("frequency_penalty", None)
        query_args.pop("num_generations", None)
        query_args.pop("presence_penalty", None)
        query_args.pop("model", None)

@arnavsinghvi11
Copy link
Collaborator

Thanks @drawal1 ! looks great! just left a few minor comments but should be good to merge after that.

@drawal1
Copy link
Contributor Author

drawal1 commented Apr 11, 2024

@arnavsinghvi11 - Should be ready to merge now, hopefully :)

@arnavsinghvi11
Copy link
Collaborator

arnavsinghvi11 commented Apr 12, 2024

Thanks @drawal1 ! looks great now. Just a small thing before I merge. Could you add some further documentation for AWSProvider? Missed flagging this in earlier comments but something similar to what's done for AWSMistral and add it for an example AWSProvider like Bedrock

@drawal1
Copy link
Contributor Author

drawal1 commented Apr 12, 2024

@arnavsinghvi11 - AWSProvider is now documented as well

@arnavsinghvi11 arnavsinghvi11 merged commit bcebff1 into stanfordnlp:main Apr 12, 2024
@arnavsinghvi11
Copy link
Collaborator

Thanks for all the contributions @drawal1!

@mikeusru
Copy link

num_generations

@drawal1 it seems like num_generations is being set to None automatically if "n" and "num_generations" doesn't exist in kwargs, which causes the error in this line:
if (temperature is None or temperature <= 0.15) and num_generations > 1:

One potential fix is changing this to
if (temperature is None or temperature <= 0.15) and num_generations is not None and num_generations > 1:

@drawal1
Copy link
Contributor Author

drawal1 commented Apr 12, 2024

@mikeusru - what aws provider and model are you using? can you share a code snippet so I can repro?

@mikeusru
Copy link

Using Bedrock and claude3-haiku

bedrock = dspy.Bedrock(...)

haiku = dspy.AWSAnthropic(
    aws_provider=bedrock,
    model="anthropic.claude-3-haiku-20240307-v1:0",
)

class BasicQA(dspy.Signature):
    """Answer questions with short factoid answers."""

    question = dspy.InputField()
    answer = dspy.OutputField(desc="often between 1 and 5 words")
    
# Define the predictor. Notice we're just changing the class. The signature BasicQA is unchanged.
generate_answer_with_chain_of_thought = dspy.ChainOfThought(BasicQA)

# Call the predictor on the same input.
pred = generate_answer_with_chain_of_thought(question=dev_example.question)

Error: TypeError: '>' not supported between instances of 'NoneType' and 'int'

The error is fixed when the change I posted earlier is introduced

@drawal1
Copy link
Contributor Author

drawal1 commented Apr 15, 2024

@mikeusru - good catch! I agree with your solution. Do you want to submit a PR? Otherwise, I can do it

@mikeusru
Copy link

mikeusru commented Apr 15, 2024

@mikeusru - good catch! I agree with your solution. Do you want to submit a PR? Otherwise, I can do it

I suppose I can submit the PR - my first ever on github :D

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.

4 participants