Re: [PR] OPENNLP-1931: Regex removal (3/10): Read opennlp-dl vocab and config JSON with a strict scanner (opennlp)

["rzo1 (via GitHub)" <gi...@apache.org>]

rzo1 commented on PR #1277:
URL: https://github.com/apache/opennlp/pull/1277#issuecomment-5743853571

   Thanks for the update. CI is green now, and the description is refreshed. I have three points on the code.
   
   **1. Please move the `tokenizer.json` support to its own issue and PR**
   
   Doing it properly needs more than this PR should carry. `AbstractDL` always builds a `WordpieceEncoder` (`AbstractDL.java:111`, `:142`), yet the Javadoc (`:541`) and the Unigram error message (`:592`) say BPE models are supported, and `LoadVocabTest:259` loads a `"type": "BPE"` file. A RoBERTa `tokenizer.json` loads without error and then gets WordPiece over byte-level BPE tokens, so predictions go wrong without any error. The support needs a `model.type == "WordPiece"` gate, and possibly `continuing_subword_prefix` and the normalizer's lowercase flag as well. That's a feature of its own, not regex removal.
   
   For this PR, please:
   - drop the `tokenizer.json` commits (`912b434d`, `c1f98f5f`) and the `tokenizer.json` cases from `16c40928`;
   - remove the `tokenizer.json` sentences from `doccat.xml` and from the description;
   - have the rejection of a `tokenizer.json` say what went wrong. At the moment it would read like "`version` is not a non-negative integer". Something like "expected one object mapping tokens to integer ids, as in vocab.json" would be clearer.
   
   Nothing is released between the two PRs, since `main` is 3.0.0-SNAPSHOT, so if the follow-up lands before 3.0.0 no user ever sees the gap. Please file an issue for it.
   
   **2. Mark `JsonScan` as `@Internal`**
   
   `JsonScan` is `public` only because `DocumentCategorizerConfig` in `opennlp.dl.doccat` calls `stringObject`. `opennlp-dl` has no `module-info`, so the class becomes visible API as soon as it ships. Please annotate it with `@Internal(since = "3.0.0")` (`opennlp.tools.commons.Internal`).
   
   **3. Reference OPENNLP-1953**
   
   The byte order mark fix in `loadVocabFile` is [OPENNLP-1953](https://issues.apache.org/jira/browse/OPENNLP-1953). Please mention it in those commits or in the squash message so the issue can be closed when this merges.
   
   The eval build (`DocumentCategorizerDLEval`) is still needed before merge.
   


-- 
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: commits-unsubscribe@opennlp.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org