Separate Token Estimators For OpenAI Models#95
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #95 +/- ##
==========================================
+ Coverage 98.39% 98.40% +0.01%
==========================================
Files 10 10
Lines 992 996 +4
Branches 102 100 -2
==========================================
+ Hits 976 980 +4
Misses 7 7
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
memor/tokens_estimator.py
Outdated
|
|
||
| UNIVERSAL = universal_tokens_estimator | ||
| OPENAI = openai_tokens_estimator | ||
| OPENAI_GPT35_TURBO = openai_tokens_estimator_gpt35_turbo |
There was a problem hiding this comment.
My suggestion: OPENAI_GPT_3_5 instead of OPENAI_GPT35_TURBO
There was a problem hiding this comment.
✅ GPT_3_5 is better than GPT35 — and here's why:
✔️ Clarity & Readability
GPT_3_5 mirrors the actual model name (gpt-3.5) more closely.
Avoids ambiguity: 35 could be misread as "thirty-five" instead of "3.5".
🔢 Consistency with naming standards
Enum names typically use UPPER_SNAKE_CASE, and separating numbers with underscores (when they represent separate components) improves readability.
memor/tokens_estimator.py
Outdated
| UNIVERSAL = universal_tokens_estimator | ||
| OPENAI = openai_tokens_estimator | ||
| OPENAI_GPT35_TURBO = openai_tokens_estimator_gpt35_turbo | ||
| OPENAI_GPT4 = openai_tokens_estimator_gpt4 |
There was a problem hiding this comment.
My suggestion: OPENAI_GPT_4 instead of OPENAI_GPT4
3 participants
Reference Issues/PRs
#67
What does this implement/fix? Explain your changes.
We separated the token estimators for two models of ChatGPT3.5-turbo and 4.
Now we have
openai_tokens_estimator_gpt_3_5openai_tokens_estimator_gpt_4as our estimators instead of
openai_tokens_estimator.Any other comments?
@sepandhaghighi Note that I removed the check for non-str inputs since we're not doing it for the universal estimator, and I wanted them to be more consistent. If we needed them we can add them in a future PR.