-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Remove token-counting library for conversation history truncation #2449
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
Conversation
@@ -59,7 +59,7 @@ jobs: | |||
run: black . --check --verbose | |||
- name: Run Python tests | |||
if: runner.os != 'Windows' | |||
run: pytest -s -vv --cov --cov-fail-under=86 | |||
run: pytest -s -vv --cov --cov-fail-under=89 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know what's even better? Our Windows tests almost pass! Tiktoken was the main thing holding them back. There are about 6 that fail now due to how relative file URLs are handled with test files, but we could get those working. Anyway thats for another PR.
@@ -27,7 +27,6 @@ PyMuPDF | |||
beautifulsoup4 | |||
types-beautifulsoup4 | |||
msgraph-sdk | |||
openai-messages-token-helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
@@ -238,7 +239,7 @@ async def validate_qr_and_mock_search(*args, **kwargs): | |||
use_vector_search=True, | |||
use_semantic_ranker=True, | |||
use_semantic_captions=True, | |||
use_query_rewrites=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this test
@pamelafox won't this now cause an exception if the conversation exceeds the token limit? I know it's unlikely, but I have encountered this within my organisation especially when people are conversing about code. |
@TaylorN15 It should cause an exception that gets handled here: Have they managed to run over it with 128K tokens, or only with the older models? |
Ah I missed that, thanks. Yes I've had people go over the 128K which is pretty crazy. I'm not using this code base directly as it is, mine is highly customised (I.e I have handling in CosmosDB for conversation that exceed 2MB, caused by this same issue). |
@TaylorN15 Good to know! Btw, we did recently change the CosmosDB to only store a message per-document, to avoid hitting the 2MB limit. |
token counting helper library served this repo very well for a good period of time. I hope it finds utilisation elsewhere. RIP. |
Purpose
This PR removes the functionality that reduced conversation size in context windows by counting tokens.
Reasons for removal:
Does this introduce a breaking change?
When developers merge from main and run the server, azd up, or azd deploy, will this produce an error?
If you're not sure, try it out on an old environment.
Does this require changes to learn.microsoft.com docs?
This repository is referenced by this tutorial
which includes deployment, settings and usage instructions. If text or screenshot need to change in the tutorial,
check the box below and notify the tutorial author. A Microsoft employee can do this for you if you're an external contributor.
Type of change
Code quality checklist
See CONTRIBUTING.md for more details.
python -m pytest
).python -m pytest --cov
to verify 100% coverage of added linespython -m mypy
to check for type errorsruff
andblack
manually on my code.