rzo1 commented on PR #1165: URL: https://github.com/apache/opennlp/pull/1165#issuecomment-5113565122
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<String> 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
