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

Refactor transform_messages #1631

Merged
merged 15 commits into from
Feb 20, 2024
Merged

Conversation

dkirsche
Copy link
Contributor

Why are these changes needed?

Some small refactors to improve readabilty, simplify and optimize.

Improvements

  • The refactored code reduces the overall complexity and the number of iterations over the messages.
  • Consolidated Truncation Logic: The logic for truncating each message to max_tokens_per_message and trimming the list to max_messages is more straightforward and easier to follow. It consolidates several steps into fewer lines of code.
  • Total tokens count excludes system to be apples to apples comparison to processed_messages_tokens
  • truncate_str_to_tokens requires less iterations. Iterate over tokens instead of characters.

Related issue number

None

Checks

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2750391) 39.33% compared to head (350aa4a) 61.97%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1631       +/-   ##
===========================================
+ Coverage   39.33%   61.97%   +22.63%     
===========================================
  Files          57       57               
  Lines        6096     6093        -3     
  Branches     1365     1481      +116     
===========================================
+ Hits         2398     3776     +1378     
+ Misses       3502     1974     -1528     
- Partials      196      343      +147     
Flag Coverage Δ
unittests 61.75% <100.00%> (+22.42%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dkirsche
Copy link
Contributor Author

@dkirsche please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@dkirsche
Copy link
Contributor Author

@dkirsche please look at the commits I submitted here: dkirsche#1

Especially pay attention to the second commit which added a new test case and fixed a bug. Lmk what you think!

Your changes made sense. I pulled them into the PR.

@gagb
Copy link
Collaborator

gagb commented Feb 20, 2024

@dkirsche please look at the commits I submitted here: dkirsche#1
Especially pay attention to the second commit which added a new test case and fixed a bug. Lmk what you think!

Your changes made sense. I pulled them into the PR.

Thank you so much @dkirsche! Also, I think we need to improve this function to handle the case when the message history has tool_calls and responses. But we can fix this in a new PR.

@dkirsche
Copy link
Contributor Author

Some of the checks are in a waiting state. @ekzhu I believe I need your review before proceeding.

@sonichi sonichi added this pull request to the merge queue Feb 20, 2024
Merged via the queue into microsoft:main with commit a34e4cc Feb 20, 2024
52 of 57 checks passed
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
* refactored code to simplify

* optimize function. Instead of iterating over each character, guess at size and then iterate by token.

* adding tests

* Add missing tests

* minor test fix

* simplified token truncation by using tiktoken to encode and decode

* updated truncated notification message

* Fix llm_config spec to use os.environ

* Add test case and fix bug in loop

---------

Co-authored-by: gagb <gagb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
long context handling Compression to handle long context
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants