Skip to content

[Exception Handling] DeepSeek JSONDecodeError #29758

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

Merged
merged 12 commits into from
Feb 23, 2025

Conversation

AhmedTammaa
Copy link
Contributor

For Context please check #29626

The Deepseek is using langchain_openai. The error happens that it show json decode error.

I added a handler for this to give a more sensible error message which is DeepSeek API returned empty/invalid json.

Reproducing the issue is a bit challenging as it is inconsistent, sometimes DeepSeek returns valid data and in other times it returns invalid data which triggers the JSON Decode Error.

This PR is an exception handling, but not an ultimate fix for the issue.

@dosubot dosubot bot added size:M bug Related to a bug, vulnerability, unexpected error with an existing feature labels Feb 12, 2025
Copy link

vercel bot commented Feb 12, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Feb 23, 2025 7:47pm

Copy link
Collaborator

@ccurme ccurme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a problem with DeepSeek. I don't think we should handle this in langchain-openai (there are many providers who subclass BaseChatOpenAI, it's not tenable to account for these possibilities for all providers).

What do you think about implementing this on ChatDeepSeek-- e.g., overriding _stream and other methods and handling exceptions when you call super()._stream?

@ccurme ccurme self-assigned this Feb 13, 2025
@AhmedTammaa
Copy link
Contributor Author

Aha, thank you @ccurme, I didn't know that it is used by others other than Openai and Deepseek. You are absolutely right about your concern. I will make the suggested changes and revert the BaseChatOpenAI changes.

@AhmedTammaa AhmedTammaa changed the title DeepSeek Exception Handling OpenAIBase [Exception Handling] DeepSeek JSONDecodeError Feb 14, 2025
@AhmedTammaa
Copy link
Contributor Author

@ccurme I have made the changes as suggested. However, I need an advice here: Should I raise the error as ValueError as we have it from the parent class or JSONDecodeError?

please note I did a small refactor in the function _create_chat_result the ruff formatter was moving the type ignore comment so it was triggering the linting checker.

Copy link
Collaborator

@ccurme ccurme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment on lines 260 to 261
except ValueError as e:
raise ValueError(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we catch ValueError, or JSONDecodeError?

In this commit we were catching JSONDecodeError: 4bd23d6

I think we should raise the same error type as is emitted now (e.g., JSONDecodeError, if that's what the client raises).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect thank you very much. I was very perplexed here. I will update it

@AhmedTammaa
Copy link
Contributor Author

I handled the exception as JSONDecodeError as this the triggered exception.

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Feb 23, 2025
@ccurme ccurme merged commit 8b511a3 into langchain-ai:master Feb 23, 2025
20 checks passed
@AhmedTammaa AhmedTammaa deleted the deep-seek-hot-fix branch February 23, 2025 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Related to a bug, vulnerability, unexpected error with an existing feature lgtm PR looks good. Use to confirm that a PR is ready for merging.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants