Skip to content

Reject unimplemented BPE options instead of ignoring them silently - #2

Open
bjo4 wants to merge 1 commit into
OpenFormosa:mainfrom
bjo4:fix/fail-loud-unsupported-bpe-flags
Open

Reject unimplemented BPE options instead of ignoring them silently#2
bjo4 wants to merge 1 commit into
OpenFormosa:mainfrom
bjo4:fix/fail-loud-unsupported-bpe-flags

Conversation

@bjo4

@bjo4 bjo4 commented Jul 28, 2026

Copy link
Copy Markdown

Refs #1

BPE and BpeTrainer accept a set of HuggingFace-compatible options, store
them, and in the model's case serialize them back out, without ever acting on
any of them. Setting one gives you tokenization that differs from HuggingFace
with no signal, and a round-tripped tokenizer.json looks unchanged, so nothing
appears wrong.

This PR makes the ones I am not implementing fail loudly. The four that change
token output — unk_token, fuse_unk, byte_fallback, ignore_merges — are
implemented in the follow-up PR, which is stacked on this branch.

What changes

Option Accepted Rejected
dropout None, 0.0 (0, 1]UnsupportedFeatureError; outside [0, 1]ModelError
continuing_subword_prefix None, "" any non-empty string → UnsupportedFeatureError
end_of_word_suffix None, "" any non-empty string → UnsupportedFeatureError

Defaults stay silent on purpose. None, 0.0 and "" are genuine no-ops in
HuggingFace too — I checked — so files that tokenize correctly today keep
loading, and GPT-2-lineage exports ship "" for both affix options. An
out-of-range dropout is invalid rather than unimplemented, which is why it is
ModelError; HuggingFace also rejects it at construction.

BpeTrainer gets the same treatment for its two affix options, which it also
stored and never read. Fixing only the model would have left half of the same
defect in place, and the inconsistency would be the first thing a reviewer noticed.

cache_capacity was being discarded entirely — models/bpe.py:41 accepted it
without even an attribute assignment. It is now retained but not rejected,
because a cache is a pure optimization: its absence cannot change output. There
is a test asserting it stays out of to_dict(), since HuggingFace does not
serialize it into tokenizer.json either.

The rejection rules live in _validation.py rather than being duplicated,
because they have to stay identical in the model and the trainer.

Still stored and unread, deliberately out of scope

Listing these so the inventory is complete rather than looking like an oversight:

  • decoders/byte_level.pytrim_offsets, add_prefix_space, use_regex
  • pre_tokenizers/byte_level.pytrim_offsets

They are an offsets concern rather than a token-identity one, and they live in
different components. Happy to take them in a separate PR if you want them.

Testing

25 new tests in tests/test_unsupported_flags.py, covering each option's
accepted and rejected values, the exception hierarchy, message quality, the
cache_capacity retention and serialization behaviour, and the
Tokenizer.from_dict path.

Full suite: 78 passed (53 before this change, 25 new). No existing test
changed.

BPE and BpeTrainer accepted a set of HuggingFace-compatible options, stored
them, and in the model's case serialized them back out, without ever acting on
any of them. A caller who set one got wrong tokenization with no signal, and a
round-tripped tokenizer.json looked unchanged, so nothing appeared wrong.

Options that would change tokenization now raise UnsupportedFeatureError:
dropout in (0, 1], and non-empty continuing_subword_prefix / end_of_word_suffix.
A dropout outside [0, 1] raises ModelError instead, since that is invalid rather
than unimplemented, matching HuggingFace's own rejection at construction.

Default values stay silent. None, 0.0 and "" are genuine no-ops in HuggingFace
too, so files that tokenize correctly today keep loading -- GPT-2-lineage
exports ship "" for both affix options.

BpeTrainer gets the same treatment for its two affix options, which it also
stored and never read. Fixing only the model would have left half of the same
defect in place.

cache_capacity was being discarded entirely, without even an attribute
assignment. It is now retained, but not rejected: a cache is a pure
optimization, so its absence cannot change output. HuggingFace does not
serialize it into tokenizer.json, so to_dict() stays faithful by omitting it.

The rejection rules live in a shared module because they must stay identical in
the model and the trainer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants