Skip to content

OPENNLP-1885: Add subword API and WordPiece encoder - #1165

Open
krickert wants to merge 3 commits into
apache:mainfrom
ai-pipestream:sentencepiece
Open

OPENNLP-1885: Add subword API and WordPiece encoder#1165
krickert wants to merge 3 commits into
apache:mainfrom
ai-pipestream:sentencepiece

Conversation

@krickert

@krickert krickert commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • adds the SubwordTokenizer contract to opennlp-api
  • adds SubwordPiece with exact UTF-16 spans into the caller's original text
  • adds a dependency-free WordpieceEncoder for BERT-style vocabularies
  • keeps BertTokenizer as a deprecated compatibility class
  • documents subword tokenization and original-text offset behavior

This PR contains no SentencePiece model reader, model normalizer, or inference
engine. The concrete SentencePiece implementation is
apache/opennlp-addons#178.

API

SubwordTokenizer.encode(text) returns List<SubwordPiece>.
encodeToIds and encodeToPieces provide the corresponding compact views.
Each SubwordPiece contains its vocabulary spelling, id, and source span.
Control and fill pieces use empty source spans.

WordpieceEncoder accepts a vocabulary, applies longest-prefix segmentation,
and preserves original-text offsets. Its maximum word length is counted in
Unicode code points.

Validation

  • opennlp-api: 367 tests passed
  • opennlp-runtime: 2,183 tests passed
  • vocabulary entries and piece ids reject invalid values
  • reference sequences, Unicode input, and source offsets are covered
  • the documentation HTML and PDF packages generated

@krickert krickert changed the title Pure-Java SentencePiece inference with exact original-text spans (opennlp-subword) OPENNLP-1885 - Pure-Java SentencePiece inference with exact original-text spans (opennlp-subword) Jul 10, 2026
@krickert krickert changed the title OPENNLP-1885 - Pure-Java SentencePiece inference with exact original-text spans (opennlp-subword) OPENNLP-1885: Pure-Java SentencePiece inference with exact original-text spans (opennlp-subword) Jul 10, 2026
@krickert krickert self-assigned this Jul 10, 2026
@krickert

Copy link
Copy Markdown
Contributor Author

Single-thread throughput measurement after 3fb8b6b, for the record.

Machine: AMD Ryzen 9 9950X3D (16 cores, one thread used), Linux, OpenJDK 25.0.3. Workload: 100,100 short texts (77 distinct lines cycled), t5-small unigram model, 32k vocabulary, 1.17M pieces total, 3 warmup passes over the corpus, then 5 timed passes.

  • opennlp-subword: 6.47M pieces/s (554k texts/s), producing piece, id, and original-text span for every token
  • Reference implementation, sentencepiece 0.2.1 via its Python binding, one encode call per text, ids only: 4.57M pieces/s (391k texts/s)
  • opennlp-subword before the optimization commit: 2.83M pieces/s

That is 1.42x the reference on the same corpus and model, measured call for call from a host language, so the binding's per-call overhead is included in the reference number; the raw C++ core inside a batch loop is faster than that number. The Java side also does more work per token, since it maps every piece back to a UTF-16 span of the original input, which the reference does not produce against the original string.

Output is unchanged: the bundled parity fixtures and the T5-small and ALBERT real-model fixtures assert identical pieces, ids, spans, and normalized forms against the reference before and after the optimization commit.

@krickert

Copy link
Copy Markdown
Contributor Author

This is now dependent on the embeddings to land. Marking ready for review

@krickert
krickert marked this pull request as ready for review July 13, 2026 05:29
@rzo1
rzo1 marked this pull request as draft July 14, 2026 12:26
@rzo1

rzo1 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

If it depends on #1152 , it should still be "draft" state (since the base PR is also draft)

@krickert

Copy link
Copy Markdown
Contributor Author

@rzo1 Correction, my earlier comment had the dependency backwards: this PR is standalone on main, and #1152 is the one that stacks on it (its base is this branch). Nothing in opennlp-subword references the embeddings module, and CI is green on the full matrix with no #1152 code involved. Marking it ready again, sorry for the churn.

@krickert
krickert marked this pull request as ready for review July 14, 2026 14:35
@krickert
krickert force-pushed the sentencepiece branch 3 times, most recently from cc74b82 to 6d40bc0 Compare July 19, 2026 11:41

@rzo1 rzo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extensive work here. Reviewed file-by-file — no blocking issues; the public API validates at the boundary with IAE, serialVersionUIDs are in order, the module is wired in, and the BertTokenizer removal is fine for a pre-release milestone. A handful of minor items to clean up before merge:

Javadoc — missing @throws

  • WordpieceEncoder.java:75 / :87 — the 1-arg and 2-arg public constructors delegate to the throwing map constructor (null/duplicate vocab, missing special token) but lack the @throws IllegalArgumentException clause the 5-arg (:106) and map (:126) constructors already carry. Please add it, matching the 5-arg wording.
  • PieceTrie.java:95build() propagates IAE on a duplicate piece but omits the @throws both Builder methods carry. Add @throws IllegalArgumentException Thrown if a piece is defined more than once.
  • DoubleArrayTrie.java:57 — constructor throws IAE when length is non-positive or not a multiple of four; add @throws IllegalArgumentException Thrown if length is not a positive multiple of four.
  • DoubleArrayTrie.java:79longestPrefixMatch() maps an out-of-range unit reference (corrupt data) to IAE with no @throws; please document it.

Constants

  • ModelProtoReader.java:100 — the tag field-number shift (>>> 3) and wire-type mask (& 7) are raw literals repeated ~14×, while WIRE_*/FIELD_* are already named constants. Please declare TAG_FIELD_SHIFT = 3 and TAG_WIRE_MASK = 7 (or fieldOf(tag)/wireTypeOf(tag) helpers) and use them at every tag-decomposition site.
  • PieceTrie.java:70 — the 256-entry dispatch-table width is an unnamed literal at :70 and :76; only DIRECT_THRESHOLD is named. Please add DIRECT_TABLE_SIZE = 256.

Duplication

  • IntBuilder.java vs ByteBuilder.java — the 1.5× growth (data.length + (data.length >> 1)), the Math.max(capacity, 16) floor, and truncate() validation are duplicated byte-for-byte. Please name the shared 16 floor and growth policy so the two copies stay in sync. The int/byte split itself is fine as primitive specialization.

Comments

  • SentencePieceNormalizer.java:150 / :193 — "heading whitespace" / "heading spaces" should read "leading" in both comments.

Exception convention (discussion)

  • SentencePieceTokenizer.java:224 / :240load(Path) / load(InputStream) surface a malformed .model as the unchecked IllegalArgumentException, which diverges from the other OpenNLP model loaders that throw the checked InvalidFormatException for bad model content. Would you consider InvalidFormatException for content errors (keeping IAE for null-arg guards)? The class currently uses IAE uniformly by design, so flagging for discussion rather than as a fix.

Process

  • BertTokenizer shipped in the opennlp-3.0.0-M4 tag, so describing its removal as "unreleased" is slightly imprecise. Not blocking for a milestone, but please correct the wording and add a one-line migration pointer for former BertTokenizer users → WordpieceEncoder.

One note on coverage: I reviewed the parity/fixture tests for form, not by re-running them. The piece-for-piece reference assertions look right structurally, but I have not executed the suite as part of this pass.

@mawiesne

mawiesne commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

This will be projected to OpenNLP 3.0.0 (M6) - not M5

krickert added a commit to ai-pipestream/opennlp that referenced this pull request Jul 24, 2026
…ENNLP-1895 recorded

Restate the map against apache main a864230, cut as 3.0.0-M5 on 2026-07-24.
apache#1177 (OPENNLP-1870) merged upstream and moves into the merged box, apache#1190 and
apache#1191 are marked ready for review, and OPENNLP-1895 (quantized embedding
tables) joins the diagram in its own colour: filed in JIRA with the pull
request deliberately held until apache#1165 and apache#1152 move.

Statuses now carry the measured GitHub draft flag and how far each head sits
behind main, which surfaces three things the old text did not: apache#1182 is a draft
again, apache#1167 is based on main rather than on apache#1155 and carries the seam and
isBlank commits as copies, and apache#1152 reports conflicts only because its
apache-hosted sentencepiece base has diverged from the refreshed head.
@rzo1

rzo1 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This PR removes stuff which was added in a previous milestone release of opennlp. Please add some rational so reviewers get an idea why sth was dropped.

In addition, it needs to be carefully checked, which classes belong into API and which can go into a custom submoduel.

@krickert

krickert commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@rzo1 the removals are intentional; I'll provide the full rational in a few hours. I should've provided a changelog at the top of the review. It'll go over every detail.

Moving to draft until that detailed explanation lands.

@krickert
krickert marked this pull request as draft July 28, 2026 09:37
@krickert
krickert marked this pull request as ready for review July 28, 2026 22:43
@krickert

Copy link
Copy Markdown
Contributor Author

Since I pushed it, while enhancing it I didn't think it was as good of a contract.

There are two subword engines and they needed one contract. BertTokenizer could not be it: it was pinned to Tokenizer, which promises spans into the input, and wordpiece pieces are not substrings of the text. That is why its tokenizePos threw from the day I added it.

The new contract is SubwordTokenizer, returning SubwordPiece(piece, id, start, end), so the piece and the original-text span are separate fields. BertTokenizer folded into WordpieceEncoder under it, and SentencePieceTokenizer implements the same one.

Example:

WordpieceEncoder encoder = new WordpieceEncoder(vocabulary, lowerCase);
List<SubwordPiece> pieces = encoder.encode(text);   // piece, vocab id, and span
String[] tokens = encoder.encodeToPieces(text);     // what BertTokenizer.tokenize returned

The old pipeline is kept as ReferenceBertPipeline and the encoder is differential-tested against it, so the piece sequence is asserted identical.

AbstractDL.createTokenizer now returns Tokenizer but there's no in-tree caller. It is binary-incompatible for anyone overriding it.

opennlp-dl compiles against opennlp-api only and consumes wordpiece, so wordpiece stays in api. SentencePiece has no core consumer, so that engine sits in opennlp-extensions/opennlp-subword with only the contract in api.

@rzo1

rzo1 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Nothing in your argumentation force the removal: those are separable. EncoderTokenizer already is the compat shim, it's just package-private and in opennlp-dl. Three things I'd like to change:

  1. Keep BertTokenizer, reimplemented over WordpieceEncoder. @deprecated(since = "3.0.0", forRemoval = true), same constructors, tokenize() delegates to encodeToPieces(), tokenizePos() keeps throwing as it does today. The old ctor takes Set and the encoder wants ids, but ids are unused on the tokenize() path, so a synthesized index map is fine (worth a comment saying so). EncoderTokenizer then goes away and the adapter sits in opennlp-api, where downstream code can actually reach it, instead of being hidden in opennlp-dl.
  2. Drop ReferenceBertPipeline and differential-test against the real class. Right now the baseline is a test-only copy of the class being deleted. If the deprecated BertTokenizer stays, point WordpieceEncoderTest at it instead : same assertion, and it additionally pins the shim and the encoder to the same sequence. One less copy of the normalization pipeline to keep in sync.
  3. Revert AbstractDL.createTokenizer to protected BertTokenizer createTokenizer(...). You flag this yourself and it's the part that worries me most. Narrowing the return type from BertTokenizer to Tokenizer doesn't only break recompilation: an already-compiled subclass overriding it with descriptor ()Lopennlp/tools/tokenize/BertTokenizer; stops overriding at runtime, so the base implementation silently wins and the subclass's tokenizer is never used. A silent behavior change in a protected extension point is worse than a compile error. With (1) in place this reverts to a one-word change, since createPipelineTokenizer can hand back the shim.

If you'd rather see the class gone in this PR, the minimum I'd want is the adapter promoted to public API in opennlp-api plus a migration note, so Tokenizer t = new BertTokenizer(vocab, lowerCase) has a one-line replacement. But it's ~30 lines of delegation and it buys back both the source API and the binary compatibility, so I'd rather deprecate now and remove in 3.1, after it has been deprecated through one stable release.

The encoder itself and the span mapping look good - this is only about how we retire the old entry point.

@krickert

Copy link
Copy Markdown
Contributor Author

No problem! On it now.

@krickert

Copy link
Copy Markdown
Contributor Author

All three are in.

  1. BertTokenizer is back in opennlp-api as a shim over WordpieceEncoder: @Deprecated(since = "3.0.0", forRemoval = true), the original three constructors, tokenize() delegating to encodeToPieces(), tokenizePos() throwing with the original message. Ids are synthesized from the set order, with the comment, since the tokenize() path never reads them. EncoderTokenizer is gone.
  2. ReferenceBertPipeline is gone; the curated and randomized differential tests now run against the shim, and a new BertTokenizerTest pins the constructors, the default special token chain, and the exact tokenizePos message. The independent expected sequences stay in WordpieceEncoderReferenceSequencesTest.
  3. AbstractDL.createTokenizer returns BertTokenizer again, so the old override descriptor holds, and createPipelineTokenizer hands back the shim.

The compatibility check before deleting the old pipeline surfaced a real divergence: the encoder kept U+2028 and U+2029 inside words while the old WhitespaceTokenizer split on them, so a word carrying a line or paragraph separator collapsed to [UNK]. Fixed in cleanAndIsolateCjk (Zl and Zp map to a space now, matching reference BERT's str.split()), with a span-asserting regression test. The old fuzz pool never contained those characters, which is how it survived the differential tests.

One deliberate drift to follow convention, documented in the @throws clauses: the shim rejects nulls with IllegalArgumentException rather than the old Objects.requireNonNull NPE, and it fails at construction when a special token is missing from the vocabulary instead of tokenizing toward unmappable pieces. Both follow the null-contract convention this branch was reviewed to. Say so if you want the old NPE behavior kept instead.

* state. Out-of-range unit references, which a well-formed trie never produces, fail loudly
* rather than reading arbitrary memory.</p>
*
* @see <a href="https://github.com/s-yata/darts-clone">Darts-clone</a>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is BSD-2 licensed. What of this impl is a copy? Is this a re-impl from the repo? Plz ellaborate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a re-implementation of the reader side only, written against the serialized unit layout (32-bit little-endian words: label in the low 8 bits, has-leaf in bit 8, offset in bits 10-30 with a bit-9 scale flag, leaf flag in the sign bit). Nothing from the darts-clone builder exists here; the class is 140 lines of read-only longest-prefix walk, and the API, naming, error handling, and comments are original.

The structure comes from the published literature rather than the BSD source: the trie is Aoe's double-array (IEEE TSE 1989), the unit encoding is the compact static variant of Yata et al. (IPM 2007), and the two-kinds-of-offset scheme behind the bit-9 flag is described as darts-clone's technique in Kanda et al. (SPE 2023). darts-clone is Yata's own implementation of his papers, so any correct reader of this format decodes the same bits the same way.

While re-checking this I found one expression that had converged on upstream's exact branchless form (the offset decode); 9670c34 rewrites it as the plain conditional derived from the bit layout and adds the Aoe/Yata/Kanda citations to the class javadoc alongside the existing darts-clone @see, so nothing textually coincides with the BSD source.

Given that, I don't believe a BSD-2 notice is required, but if you'd rather have one in LICENSE out of caution I'll add it.

* See the License for the specific language governing permissions and
* limitations under the License.
*/
package opennlp.subword.sentencepiece;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a more general data structure. might live in api?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd caution as it's more format-specific than it looks: it's a reader for the darts-clone serialized layout, not a general trie you can build from arbitrary keys. That means that the API is shaped around PieceTrie's byte-matching. Since it's package-private, promoting it to api later is a compatible change if a second consumer appears. Feels like a premature optimization that we don't have to standardize on at this moment.

Exposing it now would commit us to this shape. Let's keep it package-private in this PR; if a second consumer shows up, moving it to api is a follow-up.

import java.util.Arrays;

/** A growable int buffer supporting append, indexed read, and truncate. */
final class IntBuilder {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a more general data structure. might live in api?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a deliberately minimal growable int buffer that exists to avoid boxing on the hot path, without the bounds/safety a public utility would need. Same reasoning as DoubleArrayTrie: package-private today means we can promote it later without breaking anything, so I'd wait for a second consumer.

* 256-entry direct table and narrow nodes scan a short sorted label slice; both layouts enumerate
* identical transitions.</p>
*/
final class PieceTrie implements Serializable {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a more general data structure. might live in api?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is SentencePiece-specific. It maps UTF-8 byte sequences to piece ids using the model's vocabulary conventions, so I don't think it generalizes. I'll keep it package-private for now - same reasoning.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this? There is it from?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a third-party model. It's trained in-tree from corpus.txt by gen_fixtures.py using the reference sentencepiece Python package. 632d996 added a README next to the fixtures covering what gets generated, the venv steps to regenerate, and the TSV format; d8b0449 expands it into a full validation tutorial and links it from the manual's SentencePiece section.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated alongside the model by gen_fixtures.py: it encodes the fixture inputs with the reference sentencepiece implementation and records the expected pieces, ids, UTF-16 spans, and normalized form. So the oracle is the reference implementation, not the Java code under test. Format, regeneration, and validation steps are in the README (632d996, expanded in d8b0449).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the other TSVs: written by gen_fixtures.py from the reference sentencepiece implementation's output for the trained tiny model. See the README (632d996, expanded in d8b0449).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how was this generated? source?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trained in-tree by gen_fixtures.py from corpus.txt plus the multilingual add-on lines in the script, using the reference sentencepiece Python package. The README added in 632d996 has the exact regeneration commands, and d8b0449 adds the end-to-end validation walkthrough.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a short readme on how this is used or executed? (venv, etc?) - same for the other python stuff.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 632d996: README.md next to the fixtures with the venv setup, the regeneration command, what each output file is, and the TSV escape format. It also covers gen_real_fixtures.py, which produces the same TSVs for local eval against published models; those are not bundled. d8b0449 expands it into a validation tutorial (regenerate, run the parity suite, optional real-model eval) and links it from the manual's SentencePiece section.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is about the boundary. byteToChar plus SentencePieceNormalizer.Normalized#normToOrig is effectively a second alignment mechanism alongside opennlp.tools.util.normalizer.Alignment / AlignedText. They can't share code today (char↔char vs. normalized-byte → original-byte → char), so I'm not asking for unification here.

Is it intentional that both stay package-private — i.e. we're not committing to a second public alignment model, and consumers get offsets only via SubwordPiece spans? If so, can the class javadoc say that? It keeps the next reader from reading this as duplication of the normalization work merged under OPENNLP-1868/1875/1876.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's intentional. We're not committing to a second public alignment model: both stay package-private and consumers only ever see offsets through SubwordPiece spans. The class javadoc now states this (632d996) so it doesn't read as duplication of the OPENNLP-1868/1875/1876 alignment work.

@rzo1

rzo1 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

The PR puts the contract (SubwordTokenizer, SubwordPiece) and one implementation (WordpieceEncoder) into opennlp-api, but keeps SentencePieceTokenizer and all its machinery in opennlp-subword. What's the rule that decides the split? If WordPiece earns a place in the API, it's not obvious why SentencePiece doesn't.

Independently of that, a few classes in the module look general rather than SentencePiece-specific, and are the ones most likely to get reinvented elsewhere:

  • Utf8Text — UTF-16↔UTF-8 encoding with a byte→char offset map. Nothing about it is SentencePiece-bound; any byte-space model (byte-level BPE, ONNX tokenizers) needs exactly this, and it would sit naturally next to Span in opennlp.tools.util.
  • DoubleArrayTrie / PieceTrie — general data structures with no equivalent in core today.
  • ByteBuilder / IntBuilder — growable primitive buffers; core has none, so the next module that needs them will write them again.

The module only depends on opennlp-api, so moving any of these up is mechanically clean.

@krickert

krickert commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up in 632d996.

Added a README under the SentencePiece test fixtures covering regeneration of the tiny models and optional real-model fixture scripts. Utf8Text javadoc clarifies it is the encode-path byte-to-char span bridge behind SubwordPiece offsets, not a second public alignment API.

@krickert

Copy link
Copy Markdown
Contributor Author

The rule is compatibility, not taste. BertTokenizer is existing public API, deprecated in place as a shim delegating to WordpieceEncoder, and opennlp-api cannot depend on an extension module, so the delegate lives in api. It is one self-contained class. SentencePiece has no compatibility anchor and brings eleven classes including a binary model-format reader, so it stays in opennlp-subword: api carries the contract plus what existing compatibility forces; implementations with real machinery go to extensions.

On Utf8Text / DoubleArrayTrie / PieceTrie / ByteBuilder / IntBuilder: agreed they look general, but 3.0 freezes whatever we expose and each is shaped by one consumer today (the trie reads the darts-clone serialized layout; the buffers skip the bounds checks a public utility needs). Promoting them once a second consumer validates the shape is a compatible change, so they stay package-private in this PR.

If you feel strongly about either move, say so and I'll do it here; otherwise this is ready for another look.

krickert added a commit that referenced this pull request Aug 15, 2026
krickert added a commit that referenced this pull request Aug 30, 2026
@krickert krickert changed the title OPENNLP-1885: Pure-Java SentencePiece inference with exact original-text spans (opennlp-subword) OPENNLP-1885: Add subword API and WordPiece encoder Sep 3, 2026
@krickert

krickert commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Per our DEV conversation, I have slimmed this one down a bit so the rest of the impl can be in an addon

apache/opennlp-addons#178

The API portion is here.

krickert added a commit that referenced this pull request Sep 4, 2026
@rzo1

rzo1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Here are some load bearing ( ;-) ) comments:

Blocking

1. WordpieceEncoder.java:477 — case-mapping regression vs. both the reference and 3.0.0-M5.
StringUtil.toLowerCase is a simple, per-code-point mapping (Character.toLowerCase(int)). The BertTokenizer.normalize this replaces used String.toLowerCase(Locale.ROOT), and the Python reference uses str.lower() — both full mappings. Verified divergence:

"ΣΟΦΟΣ"  full → σ ο φ ο ς (U+03C2)   per-codepoint → σ ο φ ο σ (U+03C3)

So Greek text ending in Σ now tokenizes differently from HuggingFace and from what shipped in M4/M5, while the class Javadoc (WordpieceEncoder.java:36-53) claims reference-pipeline parity and links tokenization.py. Please lower case the whole run once and map offsets onto the result, or state the divergence explicitly. Note that WordpieceEncoderTest#testCodePointCaseMappingPreservesSourceRange currently pins the wrong value (vocabulary entry σοφοσ).

2. WordpieceEncoderTest#testPieceSequenceMatchesBertTokenizerOnCuratedInputs / #...OnRandomInputs — the parity tests are tautological.
BertTokenizer.tokenize() is return encoder.encodeToPieces(text), so both tests compare the encoder against itself; the 800 random rounds assert nothing. That is exactly why (1) slipped through. Please replace with expectations pinned from the reference implementation (as WordpieceEncoderReferenceSequencesTest does) or with sequences captured from the M5 BertTokenizer. The span-invariant checks embedded in the random test are the only non-vacuous part — keep those, in their own test.

3. AbstractDL.java:259 — the new API has no real caller; DL still routes through the deprecated shim and throws the ids away.
createPipelineTokenizer builds a BertTokenizer from vocab.keySet(), so ids get reassigned by arbitrary Set iteration order and are then discarded. DocumentCategorizerDL:413, NameFinderDL:826 and SentenceVectorsDL:155 each re-look-up every token via vocab.get(token) — which is precisely encodeToIds. AbstractDL already holds the real Map<String,Integer>, and WordpieceEncoder has a Map constructor for exactly this. Using it, plus encodeToIds, also removes every @SuppressWarnings("removal") in this PR.

4. AbstractDL.java:253protected BertTokenizer createTokenizer(...) returns a forRemoval type from protected DL API. Please return Tokenizer, or drop the overload.

5. BertTokenizer.java:76 — the "compatibility class" is not compatible.
Special tokens must now be present in the vocabulary (WordpieceEncoder.requiredId). new BertTokenizer(Set.of("the", "fox")) worked in M4/M5 — the deleted testCustomSpecialTokens did exactly that — and now throws IAE at construction. A deprecated shim must not change behavior: either keep it lenient, or make this a removal rather than a deprecation.

6. BertTokenizer.java:47 — the deprecation decision is not made.
@Deprecated(since = "3.0.0", forRemoval = true) sits on a class introduced in 3.0.0 (e7e1189, OPENNLP-1837) that has only ever shipped in M4/M5, and there is no other forRemoval in the codebase. Preference: delete it before GA — no pre-GA API deserves a shim plus suppression annotations across two modules. If it stays, name the removal version.

7. WordpieceEncoder.java:61 — inconsistent siblings in the same package.
MAX_WORD_CHARACTERS is hard-coded at 100 with no constructor, while WordpieceTokenizer exposes the same limit as a constructor parameter defaulting to 50, counted in UTF-16 chars, where this counts code points. Three divergences between two classes users have to pick between.

8. Partial validation alignment.
This PR gives WordpieceEncoder and BertTokenizer full IAE validation, but WordpieceTokenizer.java:119 still does this.vocabulary = vocabulary; with no checks at all. Either align all tokenizers in the package or drop it.

Minor

  • WordpieceEncoder.java:63,164,284 — the vocabulary Set is a redundant copy of ids.keySet(), and contains() + get() is two hash lookups per candidate in the inner longest-match loop. One Integer id = ids.get(s); if (id != null) does it.
  • WordpieceEncoder.java:282-284new String(chars, ...) plus CONTINUATION_PREFIX + substring is allocated per candidate length per word, i.e. O(n²) strings for an OOV word. Hot path.
  • WordpieceEncoder.java:471-495Character.toChars + new String + StringUtil.toLowerCase + Normalizer.normalize + a StringBuilder, all per code point. Please do this per run, and add a Normalizer.isNormalized fast path.
  • WordpieceEncoder.java:443 — the run splitting in lowerCaseAndStripAccents is dead complexity: transformRun ignores the from/to boundaries and works code point by code point. Drop the split.
  • WordpieceEncoder.java:465-470 — the Javadoc says ranges fall back to "the run's full range otherwise"; there is no such branch. Javadoc must describe the code.
  • WordpieceEncoder.java:149,188new HashMap<>(size * 2) is magic sizing. We are on Java 21: HashMap.newHashMap(size).
  • WordpieceEncoder.java:213 — parameter ids shadows the field ids, which is already assigned at both call sites. Drop the parameter.
  • WordpieceEncoder.java:395-403isLineOrParagraphSeparator is character classification and belongs in BertNormalization next to isControl/isWhitespace/isCjk/isPunctuation. Its four-line Javadoc arguing with the reference implementation should be one line plus the JIRA pointer.
  • WordpieceEncoder.java:415 — second copy of BertNormalization.isolatePunctuation. Offset tracking justifies it, but please add a pointer comment that the two must stay in sync.
  • SubwordPiece.java:36, SubwordTokenizer.java:31, WordpieceEncoder.java:54 — missing @since 3.0.0; we use it on new API (see Document.java:48).
  • SubwordTokenizer.java:37,46,62 — "@return ... empty when no units can be encoded", but WordpieceEncoder never returns fewer than two pieces. The interface says nothing about CLS/SEP framing, so encodeToIds is not portable across implementations. Please decide whether framing is part of the contract and document it there.
  • SubwordPiece.java:60span() allocates per call and duplicates the record's own start/end. Given Span is right there, either hold a Span or drop the accessor.
  • BertTokenizer.java:9 — the license-header re-indent and the removed blank line after it are unrelated churn; please revert.
  • BertTokenizer.java:88tokenize now throws IAE for null (was NPE). {@inheritDoc} alone is not enough, add @throws IllegalArgumentException.
  • AbstractDL.java:245-258@return A configured {@link BertTokenizer} on a method whose return type is being removed. Also, "pipeline tokenizer" is invented vocabulary; createBertPipelineTokenizer or plain createTokenizer reads better.
  • WordpieceEncoderTest#testValidationRejectsInvalidInput — nine assertThrows in one method, so a failure does not identify the input. Please use @ParameterizedTest. Also java.util.Map.of(...) is written fully qualified inline — import it.
  • WordpieceEncoderTest — magic seed 42 and 400 hand-rolled rounds; and the comment "The Turkish dotted capital I: lower cases to two chars, then the dot strips away" describes String.toLowerCase, not this code — Character.toLowerCase(U+0130) yields i directly.
  • tokenizer.xml:552 — the docs point users at a SentencePiece implementation in opennlp-addons, but apache/opennlp-addonsOPENNLP-1040: Add OntoNotes4 training data verification #178 is not merged. Please drop the forward reference until it lands.
  • WordpieceEncoder.java:47@ThreadSafe plus a prose repetition of it, and the fields are mutable HashSet/HashMap. Set.copyOf/Map.copyOf if immutability is being claimed.

Process

Add a general subword tokenizer contract with original-text UTF-16 offsets,
plus a dependency-free WordPiece implementation and BERT compatibility layer.
Document the API and cover vocabulary validation, reference sequences, Unicode,
and offset behavior.

Red evidence:
- A supplementary-plane word at 100 code points was rejected because UTF-16 code units were counted.
- Negative piece ids and empty vocabulary pieces were accepted.
Red evidence on the prior implementation: Greek final sigma and Unicode control categories did not match the BERT reference sequence. The sibling tokenizers also disagreed on the 100-code-point limit and model callers rebuilt non-contiguous ids.
Remove the pre-release BERT wrapper, preserve source offsets through reference-compatible normalization, and pass explicit vocabulary ids through the ONNX model callers. Align both tokenizers on Unicode code-point limits and document the public subword contract.
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.

3 participants