fix: consolidate model context-window tables (close eval gpt-5/gpt-4.1 gap)#3194
Conversation
…4.1 gap (fixes #3192) Build eval/tokens DEFAULT_CONTEXT_LENGTHS from the canonical context/budgeter.MODEL_LIMITS plus eval-specific extras, so gpt-5 and gpt-4.1 resolve to their real 1M window in the eval path. All prior eval keys (dated snapshots, o1/mistral/deepseek/groq) are preserved. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesContext limit consolidation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/eval/tokens.py`:
- Around line 76-80: Update DEFAULT_CONTEXT_LENGTHS after merging the canonical
and evaluation limits so its entries are ordered by descending key length.
Preserve all existing values while ensuring get_context_length checks specific
model names such as “gpt-4-32k” before shorter prefixes such as “gpt-4”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 035865e5-3ae1-4bc6-9e01-798002e16350
📒 Files selected for processing (1)
src/praisonai-agents/praisonaiagents/eval/tokens.py
| DEFAULT_CONTEXT_LENGTHS: Dict[str, int] = { | ||
| **{k: v for k, v in _CANONICAL_LIMITS.items() if k != "default"}, | ||
| **_EVAL_EXTRA_LENGTHS, | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sort the merged dictionary by key length to fix partial matching precedence.
Because _CANONICAL_LIMITS is unpacked first, general prefixes like "gpt-4" appear before specific models like "gpt-4-32k" (from _EVAL_EXTRA_LENGTHS) in the resulting dictionary. During partial matching in get_context_length, "gpt-4" in "gpt-4-32k-0314" will evaluate to True before the loop ever reaches "gpt-4-32k", incorrectly returning 8192 instead of 32768.
Sort the dictionary keys by length in descending order so that the longest, most specific model names are always checked first.
🐛 Proposed fix
-DEFAULT_CONTEXT_LENGTHS: Dict[str, int] = {
- **{k: v for k, v in _CANONICAL_LIMITS.items() if k != "default"},
- **_EVAL_EXTRA_LENGTHS,
-}
+DEFAULT_CONTEXT_LENGTHS: Dict[str, int] = dict(
+ sorted(
+ {
+ **{k: v for k, v in _CANONICAL_LIMITS.items() if k != "default"},
+ **_EVAL_EXTRA_LENGTHS,
+ }.items(),
+ key=lambda item: len(item[0]),
+ reverse=True,
+ )
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DEFAULT_CONTEXT_LENGTHS: Dict[str, int] = { | |
| **{k: v for k, v in _CANONICAL_LIMITS.items() if k != "default"}, | |
| **_EVAL_EXTRA_LENGTHS, | |
| } | |
| DEFAULT_CONTEXT_LENGTHS: Dict[str, int] = dict( | |
| sorted( | |
| { | |
| **{k: v for k, v in _CANONICAL_LIMITS.items() if k != "default"}, | |
| **_EVAL_EXTRA_LENGTHS, | |
| }.items(), | |
| key=lambda item: len(item[0]), | |
| reverse=True, | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/eval/tokens.py` around lines 76 - 80,
Update DEFAULT_CONTEXT_LENGTHS after merging the canonical and evaluation limits
so its entries are ordered by descending key length. Preserve all existing
values while ensuring get_context_length checks specific model names such as
“gpt-4-32k” before shorter prefixes such as “gpt-4”.
Greptile SummaryThis PR fixes a silent drift between two independent model→context-window lookup tables by making
Confidence Score: 5/5Safe to merge — the change is a pure reference-data consolidation with no new runtime logic, no new public API, and the only behavioural difference (gpt-5/gpt-4.1 now resolve to 1 M tokens) is the stated goal. The merge is deterministic and fully unit-tested. The assertion guard fires at module import time (no deferred risk). Sorting by descending key length strictly improves partial-match precision over the previous insertion-order iteration. No circular imports are introduced, and the canonical table in budgeter.py is not modified, so all existing callers of that path are unaffected. No files require special attention; the single changed file is self-contained. Important Files Changed
Reviews (2): Last reviewed commit: "fix: order eval context table by key len..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3194 ✅
Phase 1 — Architecture Review (per AGENTS.md)
Phase 2 — Fixes ImplementedBoth reviewer findings were VALID and are now fixed — commit
Skipped: Nothing. Qodo review was paused (no actionable items). No independent issues found beyond the two above. ValidationAlso verified: 49 keys total, 0 missing/changed canonical values, keys ordered descending by length. Phase 3 — Verdict✅ Approve. All reviewer findings resolved; change is minimal, backward‑compatible, and adds clear SDK value. No blocking issues remain. (Note: I can't submit a formal GitHub approval for security reasons — this comment is the review.) Job run · Branch |
Sort DEFAULT_CONTEXT_LENGTHS by descending key length so get_context_length's partial matching checks specific model names (e.g. gpt-4-32k) before shorter prefixes (e.g. gpt-4), fixing gpt-4-32k-0314 resolving to 8192 instead of 32768. Add a module-load assertion so eval-only extras can never silently shadow canonical MODEL_LIMITS entries. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
2 participants
Fixes #3192
Summary
The core SDK maintained two independent "model → context-window" tables that had drifted:
context/budgeter.MODEL_LIMITSknew the gpt-5 / gpt-4.1 (1M-context) families, buteval/tokens.DEFAULT_CONTEXT_LENGTHSdid not — so eval token accounting silently fell back to the default context length for those models.Change
eval/tokens.pynow buildsDEFAULT_CONTEXT_LENGTHSfrom the canonicalcontext/budgeter.MODEL_LIMITS(single source of truth) plus a small_EVAL_EXTRA_LENGTHStable holding eval-only keys (dated Anthropic snapshots, o1 variants, Mistral/DeepSeek/Groq,gpt-4-32k, etc.):Behaviour
get_model_limit(),get_context_length(), and theDEFAULT_CONTEXT_LENGTHfallback constant.Validation
tests/unit/eval/test_tokens.py+tests/unit/context/test_tokens.py: 44 passedtests/unit/test_token_budget.py: 29 passedGenerated with Claude Code
Summary by CodeRabbit