Repository navigation
Conversation
📝 WalkthroughWalkthroughAdds tokenizer metadata (vocab_size, chat_template_path, backend, created_at), exposes TokenizerBackend and backend() on tokenizers, updates registry load/register to accept chat_template_path, and surfaces metadata in API TokenizerInfo and related changes across crates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Summary of ChangesHello @ppraneth, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the tokenizer system by integrating comprehensive metadata directly into Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enriches the tokenizer metadata by adding backend, vocab_size, and created_at fields to TokenizerEntry and exposing them through the API. While the implementation is generally well-done, critical security concerns regarding Path Traversal and Information Exposure have been identified. As per repository rules, these cross-cutting security issues require dedicated design decisions and should be addressed in a separate, security-focused pull request. Additionally, consider improving the maintainability of the TokenizerBackend enum's string representation by providing an explicit Display implementation.
|
Thanks for the work here — the metadata enrichment is useful and the implementation is clean. But I have a fundamental concern with the API design.
|
|
Hi @ppraneth, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/core/steps/tokenizer_registration.rs`:
- Line 108: Call-site is passing caller-supplied chat_template
(chat_template.as_deref()) into tokenizer_registry.load, which lets callers
override registry metadata; remove that argument and let load() derive
chat_template_path from the loaded tokenizer instance. Specifically, in
tokenizer_registration.rs replace the .load(&id, &name, &source,
chat_template.as_deref(), || { ... }) call with .load(&id, &name, &source, || {
... }) (i.e., drop the chat_template/chat_template_path argument) and ensure any
code expecting that parameter is adjusted so metadata is computed inside
tokenizer_registry.load or the provided factory closure using the tokenizer
trait methods instead of from the caller.
| let result = app_context | ||
| .tokenizer_registry | ||
| .load(&id, &name, &source, || { | ||
| .load(&id, &name, &source, chat_template.as_deref(), || { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove caller-supplied chat_template_path from registry metadata input.
At Line 108, chat_template.as_deref() is passed into tokenizer_registry.load(...). This makes registry metadata caller-driven and can drift from the actually loaded tokenizer state (notably on remote-fallback paths). Keep load() deriving metadata from the tokenizer instance/trait and avoid passing this field at the call site.
♻️ Suggested call-site direction
- .load(&id, &name, &source, chat_template.as_deref(), || {
+ .load(&id, &name, &source, || {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/core/steps/tokenizer_registration.rs` at line 108,
Call-site is passing caller-supplied chat_template (chat_template.as_deref())
into tokenizer_registry.load, which lets callers override registry metadata;
remove that argument and let load() derive chat_template_path from the loaded
tokenizer instance. Specifically, in tokenizer_registration.rs replace the
.load(&id, &name, &source, chat_template.as_deref(), || { ... }) call with
.load(&id, &name, &source, || { ... }) (i.e., drop the
chat_template/chat_template_path argument) and ensure any code expecting that
parameter is adjusted so metadata is computed inside tokenizer_registry.load or
the provided factory closure using the tokenizer trait methods instead of from
the caller.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f98283f321
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let result = app_context | ||
| .tokenizer_registry | ||
| .load(&id, &name, &source, || { | ||
| .load(&id, &name, &source, chat_template.as_deref(), || { |
There was a problem hiding this comment.
Preserve chat template metadata only when it is actually applied
Passing chat_template.as_deref() into tokenizer_registry.load unconditionally records the requested template path even when local creation fails and the flow falls back to fetch_tokenizer_from_worker, which builds the tokenizer with None for chat templates. In that fallback scenario (common in IGW mode when the gateway cannot load locally), /v1/tokenizers and /v1/tokenizers/{id} will report a chat_template_path that was never used, so the new metadata is inaccurate.
Useful? React with 👍 / 👎.
|
Hi @ppraneth, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
|
Hi @ppraneth, this PR has been inactive for 14 days. Please update it or close it if it's no longer needed.
|
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
Problem
Closes #28
Solution
This PR enriches the
TokenizerEntrystructure and the correspondingTokenizerInfoAPI response with comprehensive metadata.TokenizerBackendenum and abackend()method on theTokenizertrait.TokenizerRegistry::loadto automatically capture:vocab_sizebackendcreated_attimestampTokenizerInfoprotocol to expose these new fields to API clients.Changes
tokenizer crate
Added
TokenizerBackendenum intraits.rs.Extended
TokenizerEntrywith:vocab_sizechat_template_pathbackendcreated_atImplemented
backend()for:HuggingFaceTokenizerTiktokenTokenizerMockTokenizerCachedTokenizerUpdated
TokenizerRegistry::loadto populate metadata automatically.Re-exported
TokenizerBackendinlib.rs.protocols crate
TokenizerInfoto include the new metadata fields.serdefeature for thechronodependency.model_gateway crate
LoadTokenizerStepto pass chat template paths correctly.TokenizerEntrymetadata toTokenizerInfo.multimodal crate
TestTokenizerto implement the newbackend()trait method.Test Plan
Added new unit tests in
tokenizer/src/registry.rsto verify metadata population and registry behavior.1. Verify Metadata Population
cargo test --package llm-tokenizer --lib registry::tests::test_metadata_populationAsserts that vocab_size is correctly captured, backend is identified as Mock, and timestamps are valid.
2. Verify Registry Immutability
cargo test --package llm-tokenizer --lib registry::tests::test_load_ignores_updatesAsserts that attempting to reload a tokenizer with different parameters does not corrupt the existing entry.
3. General Regression
Ran full test suite for tokenizer crate:
cargo test --package llm-tokenizerChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit