Skip to content
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

catch litellm no deployment error (token ratelimit / ValueError) and raise LLMProviderErrorRetryableTask #758

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Aug 29, 2024

🚀 This description was created by Ellipsis for commit 20241d5

Summary:

Handles ValueError in llm_api_handler_with_router_and_fallback by logging and raising LLMProviderErrorRetryableTask.

Key points:

  • Added ValueError exception handling in llm_api_handler_with_router_and_fallback.
  • Logs "LLM token limit exceeded" when ValueError occurs.
  • Raises LLMProviderErrorRetryableTask for ValueError.
  • Change made in skyvern/forge/sdk/api/llm/api_handler_factory.py.

Generated with ❤️ by ellipsis.dev

@wintonzheng wintonzheng merged commit 06daa98 into main Aug 29, 2024
2 checks passed
@wintonzheng wintonzheng deleted the shu/catch_llm_no_deployment_error branch August 29, 2024 19:19
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 20241d5 in 15 seconds

More details
  • Looked at 19 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/api/llm/api_handler_factory.py:113
  • Draft comment:
    The log message "LLM token limit exceeded" is misleading as ValueError can be raised for other reasons. Consider making the log message more generic or checking the specific condition that raises ValueError.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is relevant to the change made in the diff, as it addresses the new exception handling block for ValueError. The concern about the log message being potentially misleading is valid because ValueError can indeed be raised for various reasons, not just token limit issues. The comment suggests a specific improvement, which aligns with the rules for actionable and clear code quality refactors.
    The comment might be considered speculative if there is a specific context in which ValueError is only raised for token limit issues. However, without such context, the comment seems valid.
    The comment is valid unless there is specific documentation or code that guarantees ValueError is only raised for token limit issues in this context.
    The comment should be kept as it addresses a potential issue with the new code change and suggests a clear improvement.

Workflow ID: wflow_lHRslaHrvsP6ImTQ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

1 participant