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(tokenizer):add support for open source llm tokenizers #1701

Closed

Conversation

Halpph
Copy link

@Halpph Halpph commented Feb 16, 2024

Hello everyone, I saw the Contribute guide but for some reason the tests would always fail on _____________________________________ ERROR collecting test/test_function_utils.py _____________________________________ test/test_function_utils.py:288: in <module> class Currency(BaseModel): pydantic/main.py:197: in pydantic.main.ModelMetaclass.__new__ ??? pydantic/fields.py:497: in pydantic.fields.ModelField.infer ??? pydantic/fields.py:469: in pydantic.fields.ModelField._get_field_info ??? E ValueError: Fielddefault cannot be set inAnnotated for 'amount'

I tried but I'm very busy and I seem to not manage to make it work for now, I hope you can take a look and I'll try to run it again.

Why are these changes needed?

This PR solves the following issue https://github.com/microsoft/autogen/issues/1666
Basically while serving open source llm models we were always tokenizing using cl100k_base, but now we support the native way of each model by specifying it in the OAI_CONFIG_LIST

Related issue number

Closes #1666

Checks

Sadly I didn't manage to run checks because of the error mentioned above

@Halpph
Copy link
Author

Halpph commented Feb 16, 2024

@microsoft-github-policy-service agree

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (aea5bed) 39.62% compared to head (a842f46) 15.93%.

Files Patch % Lines
autogen/token_count_utils.py 27.27% 16 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1701       +/-   ##
===========================================
- Coverage   39.62%   15.93%   -23.70%     
===========================================
  Files          57       57               
  Lines        6006     6024       +18     
  Branches     1338     1457      +119     
===========================================
- Hits         2380      960     -1420     
- Misses       3433     5027     +1594     
+ Partials      193       37      -156     
Flag Coverage Δ
unittests 15.91% <27.27%> (-23.71%) ⬇️

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.

@Halpph Halpph marked this pull request as draft February 16, 2024 15:09
@sonichi sonichi requested a review from SDcodehub February 18, 2024 06:07
@sonichi sonichi added the models Pertains to using alternate, non-GPT, models (e.g., local models, llama, etc.) label Feb 18, 2024
@Halpph
Copy link
Author

Halpph commented Feb 19, 2024

@olgavrou @AaronWard @kevin666aa @SDcodehub

Hello everyone, this is a workaround that I implemented for now in order to be able to continue with my work, but I'd like to discuss with you of a proper implementation before starting the refactoring.
For instance we could implement this using some kind of strategy pattern so that the desired strategy would be responsible for fetching the right tokenizer for each case. What do you think?

Copy link

gitguardian bot commented Jul 20, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@gagb gagb closed this Aug 26, 2024
@gagb gagb reopened this Aug 28, 2024
@ekzhu ekzhu changed the base branch from main to 0.2 October 2, 2024 18:30
@ekzhu
Copy link
Collaborator

ekzhu commented Oct 2, 2024

@Halpph is this still the workaround you are using?

@Halpph
Copy link
Author

Halpph commented Oct 3, 2024

I'm not actively using it at the moment, but yes.

@jackgerrits jackgerrits added the 0.2 Issues which are related to the pre 0.4 codebase label Oct 4, 2024
@rysweet
Copy link
Collaborator

rysweet commented Oct 10, 2024

This PR is against AutoGen 0.2. AutoGen 0.2 has been moved to the 0.2 branch. Please rebase your PR on the 0.2 branch or update it to work with the new AutoGen 0.4 that is now in main.

@rysweet rysweet added the awaiting-op-response Issue or pr has been triaged or responded to and is now awaiting a reply from the original poster label Oct 10, 2024
@rysweet
Copy link
Collaborator

rysweet commented Oct 11, 2024

@Halpph If you can update to resolve conflicts and see if we can get CI to pass we can look at bringing this forward

@rysweet
Copy link
Collaborator

rysweet commented Oct 18, 2024

closing as stale, please reopen if you would like to bring it up to date.

@rysweet rysweet closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.2 Issues which are related to the pre 0.4 codebase awaiting-op-response Issue or pr has been triaged or responded to and is now awaiting a reply from the original poster models Pertains to using alternate, non-GPT, models (e.g., local models, llama, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue]: Autogen count tokens badly when using served Open Source Models
7 participants