Repository navigation
feat: add gpt-5 token counting - #12
Conversation
Test & Lint Summary
Tests: passed 7, failed 0, skipped 0 |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Implemented the core spec (structured parsing, o200k_base tokenization, image tiling, file/PDF handling, overhead) 👍
Requesting changes before merge due to a few correctness/maintainability blockers:
- Tests: several expectations are computed via the same functions under test (geometry + PDF page image tokens), which won’t catch regressions. Please switch to fixed golden numbers for representative cases.
- Tokenizer data loading: the BPE loader supports HTTP(S) downloads, which can introduce a runtime network dependency and non-determinism. Prefer bundling/embedding the ranks (or otherwise guaranteeing offline startup) per the “local counting” intent.
- PDF text extraction: current custom content-stream scanner + newline joining is fragile and likely to diverge from real extraction behavior; please either use a supported extractor (if available) or add stronger golden coverage with real PDFs + fixed expected counts.
Minor notes also left inline (image detail validation strictness, pointer/value item handling).
Test & Lint Summary
Tests: passed 11, failed 0, skipped 0 |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review: most of the prior feedback is addressed (golden numbers instead of code-under-test, strict image detail validation, pointer item handling, improved PDF whitespace handling + added tests). Thanks.
Remaining blocker:
- The new local-only BPE loader rejects the HTTPS URL that tiktoken-go passes by default. Unless
TOKEN_COUNTING_BPE_PATH/TOKEN_COUNTING_BPE_DIRis set to a realo200k_base.tiktokenat runtime (and the file is shipped), the service will error in CI/deployments. Please package/embed the full file and wire the env var in Dockerfile/Helm/devspace (and document it). The tiny test fixture is not sufficient for production accuracy.
Test & Lint Summary
Tests: passed 11, failed 0, skipped 0 |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review: BPE packaging issue is resolved.
- Docker image now downloads and bundles the full
o200k_base.tiktokenat build time and setsTOKEN_COUNTING_BPE_PATHby default. - Helm values and devspace pipelines also wire the env var / local encoding file.
LGTM.
|
BPE packaging fix is pushed (Dockerfile download + env wiring). Ready for re-review when you have a moment. |
Summary
Testing
Refs #11