dspy icon indicating copy to clipboard operation
dspy copied to clipboard

Refactor Bedrock class to support Claude-3 model and improve error handling

Open cta2106 opened this issue 1 year ago • 10 comments

This PR refactors the Bedrock class to support the Claude-3 model. The main changes include:

  1. Added support for the Claude-3 model which uses a different API format with messages instead of a single prompt. The use_messages flag is set based on the model name to determine the API format to use.

  2. Refactored the _create_body method to handle the different API formats for Claude-3 vs other models. It now constructs the appropriate request body based on the use_messages flag.

  3. Updated the _call_model method to handle the different response formats from Claude-3 vs other models. It extracts the completion text based on the expected response structure.

cta2106 avatar Mar 26 '24 02:03 cta2106

@cta2106, I encountered the same "extraneous key [n] is not permitted, please reformat your input and try again." error as you did with Claude 3. To resolve it, I adjusted the code in aws_lm.py by placing del kwargs["n"] right after the check if "n" in kwargs.keys():. I think this adjustment should address issue #680.

aazizisoufiane avatar Apr 01 '24 09:04 aazizisoufiane

Thanks @cta2106 @aazizisoufiane! Should we merge both #680 and #720 in that case? the changes make sense to me but I just want to confirm there are no behavior conflicts from merging either PR.

arnavsinghvi11 avatar Apr 01 '24 18:04 arnavsinghvi11

@arnavsinghvi11 I've tested all Claude models on bedrock, for me there is no conflict.

aazizisoufiane avatar Apr 02 '24 06:04 aazizisoufiane

Thanks folks for the contribution here, can you please just lint the code with ruff config that we have to pass the checks?

ammirsm avatar Apr 04 '24 05:04 ammirsm

Hi @cta2106,

I ran ruff check . --fix-only on my local for aws_lm.py. It seems the linting request is about bedrock.py. Could you check this on your side? Let me know if I can help.

Thanks!

aazizisoufiane avatar Apr 04 '24 10:04 aazizisoufiane

Certainement Soufiane

cta2106 avatar Apr 04 '24 11:04 cta2106

@cta2106 looks good to merge after you run ruff check . --fix-only and push!

arnavsinghvi11 avatar Apr 04 '24 21:04 arnavsinghvi11

Hey guys, please review PR https://github.com/stanfordnlp/dspy/pull/772 that tackles this issue with a much better redesign. Almost done...

drawal1 avatar Apr 05 '24 14:04 drawal1

@drawal1 is this PR good to close as well with #795 merged?

arnavsinghvi11 avatar Apr 15 '24 02:04 arnavsinghvi11

yea. pretty sure this can be closed

drawal1 avatar Apr 15 '24 03:04 drawal1