[kashif]

Overview

• Added automatic tagging of DNA in plain text
• N-padded tail tokens: a partial last k-mer is padded with N, where the vocab has these tokens
• Explicit tag handling to match HF tokenizer

Additional information

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: Yes for docs and tests

[ServeurpersoCom]

Tested carbon-14b-dna with the 14B Q8_0, the tokenizer matches the Python reference on 1256 of 1258 fuzz cases (the 2 diffs are Python Unicode quirks: str.strip eating \x1c-\x1f and ß.upper() becoming SS, not worth porting) and llama-server answers DNA questions correctly at 95 tok/s on my RTX PRO 6000, nice work!

[CISC]

/bot review

[ggml-gh-bot]

Automated code review

Review of the hybriddna tokenizer follow-up (conversion fixes + auto-tagging + N-padded tail support). I read the new session logic in src/llama-vocab.cpp, the conversion path in conversion/base.py/qwen.py, the GGUF key plumbing, and the load-time marker stripping. The design is good: the GGUF metadata key (dna_auto_tags) rather than per-model flags, a hard NotImplementedError for unsupported k/tail combos on the conversion side paired with the C++ k=6 constant, and the fuzz comparison vs the Python reference reported in the PR comments (1256/1258, 2 documented Unicode quirks) is solid evidence. I found no blocking correctness or security issues; the notes below are mostly robustness and coverage.

Will slow the review

(point 1) src/llama-vocab.cpp:2818 - the new N-strip runs on every marker-suffixed token with no guard: text.erase(text.find_last_not_of('N') + 1). If such a token were all N (crafted GGUF, or a future tail variant), find_last_not_of returns npos and erase(npos + 1) = erase(0) wipes the token to an empty string. Also note this strips trailing N unconditionally for pad_a models too - safe today only because full k-mers are ACGT-only, but that invariant is implicit. Suggest a cheap guard, e.g. only erase when find_last_not_of succeeds, plus one comment line stating the "full k-mers contain no N" invariant.

(point 2) src/llama-vocab.cpp:1894-1899 - normalize_spaced_dna is only applied when dna_auto_tags is enabled. Please confirm the reference applies _normalize_spaced_dna only inside _auto_tag_text_segment and not to all untagged segments: if HF normalizes regardless of the auto-tag flag, the older pad_a models (which this PR explicitly adds support for) will diverge on spaced DNA displays. The reported fuzz run covered Carbon-14B (auto tags on); the pad_a path appears untested.

(point 3) Test coverage: the hand-rolled regex mirrors (normalize_spaced_dna, tag splitting, the 6-base / 12-base / 20%-ambiguity thresholds) are exactly the kind of logic that regresses silently on the next touch, and none of it is covered in-repo. Consider a tests/test-tokenizer-0.sh fixture for a hybriddna vocab, or at least unit-level cases for the tag-splitting edge cases (stray </dna>, unterminated <dna>, reversed tags). Also, the new conversion fallback for the older Carbon models (no tokenizer.json, merges taken from the wrapped base tokenizer via save_pretrained into a temp dir, conversion/base.py:2226-2234) would benefit from a conversion smoke test on one of Carbon-500M/3B/8B.

Nits

(point 4) src/llama-vocab.cpp:1836 - the c != '\0' guard in is_iupac is dead: std::string("ACGTRYSWKMBDHVN").find('\0') already returns npos. Drop it.

(point 5) conversion/llama.py:132 still has the inline tokenizer_class == "HybridDNATokenizer" check that _is_hybriddna (conversion/base.py:2173) now duplicates. Since you are touching this dispatch anyway, reusing the helper there too would keep one spelling of the detection; note llama.py additionally reads add_prefix_space from the same JSON, so either return the parsed dict or leave a short note why the inline version stays.

(point 6) conversion/base.py:10 - import tempfile sits between json and os; the surrounding block is not strictly sorted, but moving it after sys would match alphabetical intent.

Questions to confirm (not blocking)

(point 7) The stray-close-tag semantics - text before an unmatched </dna> is tokenized as DNA and the close-tag token is emitted without a matching begin (src/llama-vocab.cpp:1810-1813). This is an unusual semantic; the fuzz results suggest it matches the reference, but it would help reviewers if the PR description (or a one-line comment) stated what _split_by_dna_tags does on unmatched tags, since that branch is easy to "simplify" incorrectly later.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

[CISC]

Please address bot points, sans test fixture, covered by old vocab file in ggml-org/vocabs and I will add new one as well.

[ServeurpersoCom]

Re-tested: Carbon-500M reconverts to an identical GGUF and matches the Python reference on 1023/1023 cases (the older pad_a, no auto-tag path), and Carbon-14B Q8_0 still matches on 1256/1258 (same two Python Unicode quirks), LGTM!