thanks@yiranwu0 for the changes. can you confirm if the updated code is working as expected with all the clients. have you tested it? if not@marklysze and i can give it a try.

cc@marklysze

LGTM.@marklysze could you take a look too?

Yep, I'll have a look and test now.

Thanks for raising this PR@yiranwu0, better bubbling of exceptions and centralised retries is definitely needed with all the client classes in place.

From what I understand from the code changes, the client-specific exceptions are added to client.py and then handled in thetry/except around theclient.create(params). Then it's logged and raised if it's the last client.

I was testing with Bedrock and checked out the exceptions that could be raised, and there's quite a lot of possible exceptions (boto and aws exceptions and clickClick to see a full list of static exceptions). So, I was wondering whether, rather than trying to add each client's full list of exceptions intoclient.py and catch them, would it be possible to create either a generic ClientException class or an exception class in each client class (e.g. BedrockClientException, GeminiClientException) and then raise that, if we want to handle client-specific exceptions. Other options are to create our own set of exceptions for specific common scenarios, like ClientRateLimitException, and then we raise those specific ones from the client class and any others can be generic, like ClientException.

Amazon Bedrock is probably going to be the one with the most exceptions, though Anthropic with Bedrock may also have a few. Additionally, some exceptions may have the same name, so if we continue with the proposed approach then perhaps we should prefix them, e.g.InternalServerError becomesGeminiInternalServerError.

Let me know what you think...

Hello@marklysze, thank you for testing it!
I don't think we need to attend to all exceptions from different clients, we only want to catch the few "time rate limits" exception and handle that inclient.py because we can try different configs. For other exceptions, I think we should raise the original exception, so that user can debug from that, or search online.

However, I think it is good if we can have all the rate limit exceptions wrapped in an exception of our own, so that we only need to catch one exception. But that needs more thinking and could be done in the next PR.

Thanks@yiranwu0, sounds like a plan! I'll look at the rest of the client classes and test :)

Hey@yiranwu0, I've updated the code as follows:

• Removed try/except from Ollama client
• Added main exceptions for non-OpenAI clients into client.py
• Prefixed exceptions with the client name, e.g. gemini_InternalServerError and anthorpic_InternalServerError, to avoid conflicts
• Changed variables for exceptions in ImportError to be of typeException instead ofNone asNone can't be used in an except clause. Also moved the except clause to the end of the three exception blocks.
• Tested non-function and function workflows with the following (to ensure they are still working): Anthorpic, Cohere, Groq, Mistral, Ollama, Together.AI. (I haven't tested Gemini).
• Added a note for users to also installfix-busted-json if trying to use the Ollama client.

It caught exceptions when they arose. Though I can't test all exceptions.

Hello@marklysze, your change looks good! I already tested Gemini. If there are good, we can merge it!

Hello@marklysze, your change looks good! I already tested Gemini. If there are good, we can merge it!

Okay, great... I'm good with it!