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

fix LiteLLMParams #958

Merged
merged 1 commit into from
Oct 11, 2024
Merged

fix LiteLLMParams #958

merged 1 commit into from
Oct 11, 2024

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Oct 11, 2024

Important

Convert LiteLLMParams from dataclass to TypedDict and allow None for model_info.

  • Type Change:
    • Change LiteLLMParams from dataclass to TypedDict in models.py.
    • model_info in LiteLLMParams now allows None values.
  • Imports:
    • Add TypedDict to imports in models.py.

This description was created by Ellipsis for faf9597. It will automatically update as commits are pushed.

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 faf9597 in 18 seconds

More details
  • Looked at 26 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/models.py:14
  • Draft comment:
    field is not applicable to TypedDict. Consider removing field usage from model_info in LiteLLMParams.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is correct because field is indeed not applicable to TypedDict, and the change in the diff reflects this by removing the field usage. The comment is about a change made in the diff, and it provides a clear and actionable suggestion that aligns with the change made.
    I might be missing the fact that the change has already been made, and the comment might be redundant now. The comment could be seen as unnecessary since the issue it addresses has already been resolved in the diff.
    Even though the change has been made, the comment serves as a confirmation that the change was necessary and correct. It reinforces the correctness of the change made in the diff.
    Keep the comment as it correctly identifies an issue that was addressed in the diff and confirms the appropriateness of the change.

Workflow ID: wflow_OjsIwHab9WbYzyYu


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

@wintonzheng wintonzheng merged commit 60069a6 into main Oct 11, 2024
2 checks passed
@wintonzheng wintonzheng deleted the shu/fix_LiteLLMParams branch October 11, 2024 06:24
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