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

feat: Added new max_tokens_limit values and model_names in _openai_text_completion_tokenization_details function in openai_utils.py #7316

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

tradicio
Copy link

@tradicio tradicio commented Mar 6, 2024

Related Issues

Proposed Changes:

The function _openai_text_completion_tokenization_details(model_name: str) in openai_utils.py now correctly maps the max_tokens_limit values of the gpt-3.5-turbo and gpt-4 models

How did you test it?

I have manually verified the correct max_tokens_limits in the OpenAI webpage https://platform.openai.com/docs/models/overview

Checklist

@tradicio tradicio requested a review from a team as a code owner March 6, 2024 11:32
@tradicio tradicio requested review from shadeMe and removed request for a team March 6, 2024 11:32
@CLAassistant
Copy link

CLAassistant commented Mar 6, 2024

CLA assistant check
All committers have signed the CLA.

@anakin87 anakin87 changed the base branch from v1.25.x to v1.x March 6, 2024 11:36
@anakin87
Copy link
Member

anakin87 commented Mar 6, 2024

Sorry, the PR should target the v1.x branch.
I changed the base to v1.x, but now there are some unrelated changes.

Can you rebase (or merge v1.x branch) in this PR?

@shadeMe shadeMe requested review from anakin87 and removed request for shadeMe March 6, 2024 11:44
@tradicio
Copy link
Author

tradicio commented Mar 6, 2024

I apologize for the error. I merged v1.x branch so it should be fine now!

@anakin87
Copy link
Member

anakin87 commented Mar 6, 2024

I'm trying to fix the commit history...
give me some moments...

@anakin87
Copy link
Member

anakin87 commented Mar 6, 2024

@tradicio I managed to fix the git history, to avoid issues with the CLA.

  • Some tests fail because of these new changes. Please change them.
  • Please also add a release note as explained here.

@sjrl I knew you had worked on this part in the past.
May I ask for your review?

haystack/utils/openai_utils.py Outdated Show resolved Hide resolved
haystack/utils/openai_utils.py Outdated Show resolved Hide resolved
haystack/utils/openai_utils.py Outdated Show resolved Hide resolved
@anakin87
Copy link
Member

anakin87 commented Mar 6, 2024

Oh no! The git history is broken again.
@tradicio What I think you should do is:

git remote add upstream https://github.com/deepset-ai/haystack # maybe not needed
git rebase --interactive upstream/v1.x #here, in the editor I would drop the unrelated commits
git push --force origin v1.25.x # this is your v1.25 branch

@anakin87
Copy link
Member

anakin87 commented Mar 6, 2024

@tradicio Something unexpected happened: your commits have been dropped from the git history.

If you have your code locally, just push it again, then ping me and I'll take care of fixing git history.

@tradicio
Copy link
Author

tradicio commented Mar 6, 2024

@tradicio Something unexpected happened: your commits have been dropped from the git history.

If you have your code locally, just push it again, then ping me and I'll take care of fixing git history.

Thanks for your help! Now I restored all the changes

@tradicio tradicio requested a review from a team as a code owner March 6, 2024 15:30
@tradicio tradicio requested review from dfokina and removed request for a team March 6, 2024 15:30
@anakin87 anakin87 merged commit fd13e8e into deepset-ai:v1.x Mar 6, 2024
53 checks passed
@masci masci added this to the 1.25.1 milestone Mar 22, 2024
vblagoje pushed a commit that referenced this pull request Mar 25, 2024
…xt_completion_tokenization_details function in openai_utils.py (#7316)

* Added back changes in openai_utils.py and tests are now working

* release note

---------

Co-authored-by: Tommaso Radicioni <[email protected]>
Co-authored-by: anakin87 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants