rzo1 commented on PR #1152:
URL: https://github.com/apache/opennlp/pull/1152#issuecomment-4938604187
Hi @krickert - as mentioned on Slack I currently dont have the time for a
manual review but I just let Fable do a comprehensive review on this PR. Here
is the result:
Blocking / should be addressed before merge:
- `opennlp-embeddings/src/main/java/opennlp/embeddings/TensorInfo.java`: The
public record exposes its mutable `int[] shape` component directly, so a caller
doing `tensorInfo(name).shape()[0] = 0` corrupts the metadata that
`readFloat32()` and `StaticEmbeddingModel.load()` later validate against. The
record-generated `equals`/`hashCode` also compare the array by identity, which
is surprising for exported API. `shape()` should return a defensive copy (and
the constructor copy its argument), or the component should be an immutable
representation, with `equals`/`hashCode` overridden accordingly.
-
`opennlp-embeddings/src/main/java/opennlp/embeddings/SafetensorsFile.java`: The
new public API (`StaticEmbeddingModel`, `SafetensorsFile`, `TensorInfo`,
`Neighbor`) lacks the `@Experimental` marker the codebase applies to the
analogous `WordVectorTable` API. Once released in 3.0.0 the safetensors parsing
surface becomes a compatibility commitment even though the PR lists follow-ups
(ANN index, gRPC provider) likely to reshape it. Either annotate the new public
classes `@Experimental` or narrow `SafetensorsFile`/`TensorInfo` to
package-private until the surface settles.
- `opennlp-extensions/pom.xml` / `opennlp-distr`: The new module is not
added as a dependency in `opennlp-distr/pom.xml` nor given an apidocs fileSet
in `opennlp-distr/src/main/assembly/bin.xml`, unlike morfologik, uima and
spellcheck (see #1115 and #1116). The binary distribution will ship neither the
jar nor the javadoc despite the new manual chapter documenting the module.
-
`opennlp-embeddings/src/main/java/opennlp/embeddings/WordPieceVocabulary.java`:
No dedicated test class, and the `fromLines` helper's stated purpose (letting
tests build a vocabulary from in-memory lines) is unused by any test. The
documented duplicate-token rejection, the `-1` sentinel of `id()`, and
`token(int)` bounds are all untested. Please add a `WordPieceVocabularyTest`
(which would also justify `fromLines`) or drop `fromLines` and its stale
comment.
Minor:
- `opennlp-embeddings/src/main/java/opennlp/embeddings/JsonCursor.java`:
`parseUnicodeEscape` accepts signed hex because `Integer.parseInt(hex, 16)`
tolerates a leading `+`/`-`, so `\u-0FF` is silently decoded to a wrong
character instead of being rejected. Validate the four characters are hex
digits before parsing.
- `opennlp-embeddings/src/main/java/opennlp/embeddings/JsonCursor.java`:
`skipValue`'s number branch accepts malformed tokens (a lone `-`, or junk like
`1e++--..5`) as valid numbers, contradicting the documented fail-loud contract
for malformed input.
- `opennlp-embeddings/src/main/java/opennlp/embeddings/TensorInfo.java`:
`elementCount()` can overflow `long` for tensors with three or more large
dimensions, and a wrapped small positive value can slip past `readFloat32`'s
validation for crafted headers. Use `Math.multiplyExact`-style checking.
-
`opennlp-embeddings/src/main/java/opennlp/embeddings/StaticEmbeddingModel.java`:
The load methods throw `UncheckedIOException`/`IllegalArgumentException`
instead of the checked `IOException` that every other OpenNLP model/resource
loader declares (e.g. `Glove`, `BaseModel`-style constructors). This is a
lasting API-shape decision that should be made deliberately before release.
-
`opennlp-embeddings/src/main/java/opennlp/embeddings/StaticEmbeddingModel.java`:
`load(Path, Path, boolean lowerCase, boolean normalize)` takes two adjacent
positional boolean flags; swapping them produces silently wrong embeddings.
Consider an options/builder parameter or two-value enums before the API ships.
-
`opennlp-embeddings/src/main/java/opennlp/embeddings/StaticEmbeddingModel.java`:
`embed()` pays a redundant second full `isolatePunctuation` pass per call
because `BertTokenizer.normalize` already isolates punctuation and
`WordpieceTokenizer.tokenize` re-runs it. Results are correct (the pass is
idempotent), and the code lives in the reused opennlp-api tokenizers, but it is
an optimization opportunity on the module's hot path.
-
`opennlp-embeddings/src/test/java/opennlp/embeddings/StaticEmbeddingModelConcurrencyTest.java`:
The ~30-line safetensors fixture writer is copy-pasted five times across the
tests and the JMH benchmark. A small package-private test utility (e.g.
`SafetensorsTestFiles.write(dir, name, rows...)`) would remove the duplication.
-
`opennlp-embeddings/src/test/java/opennlp/embeddings/StaticEmbeddingModelConcurrencyTest.java`:
The class javadoc claims the test "Mirrors the LexiconConcurrencyTest pattern
from the opennlp-wordnet module", but neither that test nor an opennlp-wordnet
module exists in this repository. Correct or remove the reference.
-
`opennlp-embeddings/src/main/java/opennlp/embeddings/SafetensorsFile.java`: The
`readFloat32` branch rejecting an element-count/byte-range mismatch has no
test, unlike every neighboring error branch. One small negative test (e.g.
shape `[2]` with `data_offsets [0,4]`) would pin it.
- `opennlp-embeddings/src/main/java/opennlp/embeddings/FlatJsonFields.java`
(and the rest of the module): `else` and `catch` are placed on their own line
after the closing brace, diverging from the `} else {` / `} catch (...) {`
style used throughout the existing codebase. Checkstyle does not enforce this,
but please align with the project style.
-
`opennlp-embeddings/src/main/java/opennlp/embeddings/StaticEmbeddingModel.java`:
The pooling logic is documented as "verified against" MinishLab's MIT-licensed
model2vec-rs. Please confirm no code was translated or ported from those
sources; a derived port would require an MIT attribution entry in the
distribution LICENSE file.
Human review will follow.
--
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]
