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 handling of snapshot_ids ("gpt-4-turbo-2024-04-09" and "gpt-4o-2024-05-13") and alias "gpt-4-turbo". #672

Merged
merged 3 commits into from
May 19, 2024

Conversation

st4r0
Copy link
Contributor

@st4r0 st4r0 commented May 16, 2024

Currently snapshots "gpt-4o-2024-05-13" and "gpt-4-turbo-2024-04-09" wrongfully return model costs for "gpt-4".
The same is the case for "gpt-4-turbo", which is inconsistent with the OpenAI API that has "gpt-4-turbo" pointing to "gpt-4-turbo-2024-04-09" (https://platform.openai.com/docs/models/gpt-4-turbo-and-gpt-4).

These commits would fix these issue for now. However, if OpenAI decides to change pricing so that "gpt-4-turbo-2024-04-09" and "gpt-4-turbo-preview" (pointing to "gpt-4-0125-preview") will differ in cost, the logic will not hold. I suggest adding costs for each snapshot_id and adding tests to ensure that they are correct.

If you agree, I'd be more than happy to prepare a PR.


🚀 This description was created by Ellipsis for commit 665596a

Summary:

This PR fixes incorrect cost retrieval for specific GPT-4 model snapshots and aliases, ensuring consistency with OpenAI's API.

Key points:

  • Updated get_model_cost in instructor/cli/usage.py to correctly handle costs for gpt-4-turbo-2024-04-09, gpt-4o-2024-05-13, and alias gpt-4-turbo.
  • Suggested future improvements for dynamic cost handling based on potential changes in OpenAI's pricing.

Generated with ❤️ by ellipsis.dev

Copy link

sweep-ai bot commented May 16, 2024

Sweep: PR Review

Sweep has finished reviewing your pull request.

instructor/cli/usage.py

tag. Added support for recognizing "gpt-4-turbo" and "gpt-4o" model prefixes in the get_model_cost function.

No issues found with the reviewed changes


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 665596a in 1 minute and 51 seconds

More details
  • Looked at 18 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/cli/usage.py:89
  • Draft comment:
    The alias 'gpt-4-turbo' should point to 'gpt-4-turbo-2024-04-09' as per the OpenAI API documentation, not 'gpt-4-turbo-preview'.
    elif model.startswith("gpt-4-turbo"):
        return MODEL_COSTS["gpt-4-turbo-2024-04-09"]
  • Reason this comment was not posted:
    Confidence of 80% on close inspection, compared to threshold of 85%.

Workflow ID: wflow_exELsg3cNQf2vMKP


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

@st4r0
Copy link
Contributor Author

st4r0 commented May 16, 2024

Actually I just realized that there is no way to check the actual costs the way I had imagined, so writing tests for those might not work.

- "gpt-4o-2024-05-13"
- "gpt-4-turbo"
- "gpt-4-turbo-2024-04-09"
@st4r0
Copy link
Contributor Author

st4r0 commented May 16, 2024

Now the startswith logic could be removed...

@jxnl jxnl merged commit c1aa5a1 into instructor-ai:main May 19, 2024
2 of 12 checks passed
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.

2 participants