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

[DRAFT] Make trim messages coherent #743

Closed

Conversation

hyamanieu
Copy link

@hyamanieu hyamanieu commented Nov 1, 2023

After discussing directly with the maintainers, I am suggesting this change. I can open an issue if it's necessary.

I did a bit more than just making litellm.utils.trim_messages coherent

Here are the changes:

  • now trim_messages will systematically return a tuple if return_response_tokens is set, with the second one being a number of tokens.
  • I have added type hints and docstrings where I deemed necessary
  • I have added unit tests to functions linked to trim_messages
  • I have let autoformatting on several files (commits are separated if it's an issue for you guys): utils.py, test_utils.py
  • I have allowed myself to change the filler when dumping the middle of a message in case of trimming: now it's [...] rather than ... To me LLMs are trained on large amount of human texts, and in case of partially quoting, most people (e.g. journalists) use [...] , not .. which looks more like something out of a programming language or an accountant software. I'd like to reach out to @KillianLucas to see her take on it.
  • I have allowed myself to add a new trimming strategy: rather than removing the middle, it fetches the last sentences. As the message trimming is deep into the tree starting from trim_messages, now is only the previous behavior possible. Otherwise it would have been too many chaotic changes. See functions with parameter content_trimming_strategy. Se open quetions below.
  • Poetry:
    • I have added pytest to the group "dev" as it was missing from my env
    • I have added numpydoc as test_utils.py could not run

Open questions:

  • Some unit tests did not pass, they seem related to env variables. Can you please double check or guide me?
  • It seems you're working on a large refactoring. I was thinking this "content_trimming_strategy" should be a config for the whole library after importing it rather than passed into every single function which at some point will call litellm.utils.shorten_message_content. Yet I think it can be usefull for other people to have this other strategy...
  • ... which brings me to this question: where do you want me to document litellm.utils.shorten_message_content?
  • concerning the refactoring: I believe it would be nice to make a "utils" folder and split this very large file: it's above 5000 lines!
  • What's the purpose of litellm.utils.get_token_count as it's doing nothing more than calling litellm.utils.token_counter?

Copy link

vercel bot commented Nov 1, 2023

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

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2023 5:58pm

@krrishdholakia
Copy link
Contributor

Hey @hyamanieu, appreciate the PR! the linting changes are making it hard to see the trim-message related changes made. Can you separate the 2 please?

@krrishdholakia
Copy link
Contributor

krrishdholakia commented Nov 1, 2023

Hey @hyamanieu, thanks for being responsive. I'd like to merge this but I see 8 new / changed functions in utils.py.

Can we break these into more specific PR's? that way we can test each scenario individually and ensure no breaking changes to our build pipeline are pushed.

Asking as we've spent the past few days having to deal with these issues.

@hyamanieu hyamanieu changed the title Make trim messages deterministic Make trim messages coherent Nov 2, 2023
@hyamanieu
Copy link
Author

hyamanieu commented Nov 2, 2023

After discussion with Krrish, I will split this PR in 4 PRs:

  • modify trim_messages (this PR, set in drafting mode until revamped)
  • modify the filling pattern when trimming the last message (new one)
  • add an additional strategy for trimming the last message (new one)
  • the PR with the linting and every thing else (already present here, will be reopened once the other ones are merged).

I will also move the unit tests of trim_messages from test_utils.py to the new file I have made to test its depending subfunctions.

Note: "deterministic" is not the proper word, but "coherent".

@hyamanieu hyamanieu changed the title Make trim messages coherent [DRAFT] Make trim messages coherent Nov 2, 2023
@krrishdholakia
Copy link
Contributor

you're amazing! @hyamanieu

@ishaan-jaff
Copy link
Contributor

@hyamanieu any update on this ? We have another PR for message trimming #787

@hyamanieu
Copy link
Author

I'm currently under water, I'll check back again later and see how it goes with the state of the main branch.
If my issues are then already solved, I'll simply close it.

@hyamanieu
Copy link
Author

hyamanieu commented Nov 11, 2023

Looking at the changes in #787, the issues are dealing with separate problems.
I believe his changes must be dealt with first, I will adapt my code in consequence.

@hyamanieu
Copy link
Author

I am too far off so I'm closing for now.
I'll make simpler PR in the future if I see a problem again.

@hyamanieu hyamanieu closed this Nov 26, 2023
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.

3 participants