OPENNLP-1888: Document annotation container with typed offset-anchored layers - #1182
OPENNLP-1888: Document annotation container with typed offset-anchored layers#1182krickert wants to merge 32 commits into
Conversation
4cd9beb to
7e65aad
Compare
|
This was proposed about a week ago. Discussions pointed to this shape - I feel like it's a great direction as a lot of research went into landing this shape:
|
|
I think that this needs to wait a bit more until a few more people ump into the discussion: https://lists.apache.org/thread/jwxxjkc2b0dqn4rwjvt1t7cdf056gqhp |
7e65aad to
b920e43
Compare
…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.
b920e43 to
3ab6920
Compare
…est head All nine open heads now sit directly on a864230 and report mergeable. Two were reporting conflicts and both cleared: apache#1167 through a plain rebase, and apache#1152 by pointing its apache-hosted sentencepiece base branch at the refreshed head it had drifted away from, which shrinks its diff back to the 30 commits it owns. apache#1166 shed the 13 OPENNLP-1883 commits it carried, since apache#1163 is upstream as a single squash, and is 3 commits now. Also correct what the draft flag on apache#1182 means: the branch is review-ready and waits on the upstream queue, not on unfinished work.
Review of PR #1182 — OPENNLP-1888: Document annotation container with typed offset-anchored layersBlocking
Three copies of the sentence/token walk loop.
Minor
ProcessYou have eight other open PRs (#1191, #1190, #1167, #1166, #1165, #1155, #1154, #1152). The duplication, |
|
Thanks for taking the time I will address all of these within a few hours |
819070c to
5ba76d6
Compare
|
All of it is addressed. The branch is rebased on current main. BlockingZero-length mention. Interface Javadoc. The copy-on-add and sharing narrative moved to
Three copies of the walk, and
The example contradicting the contract. The MinorDone: hoisted Reversed Entity type in two places. The annotation value is now the single source. Spans are constructed untyped and the Javadoc says so, on both Extensibility. All six adapters are Since then a second pass folded the three copies of the The document package runs 65 tests, none skipped. |
…yers over the original text Adds opennlp.tools.document to opennlp-api: Document (immutable, copy-on-add layer container over the original text), Annotation (a typed value on a Span), LayerKey (open, typed layer identity), and DocumentAnnotator (pipeline step declaring the layers it requires and provides). DocumentAnalyzer assembles annotators into a pipeline validated at build time. Standard keys in Layers cover sentences, tokens, part-of-speech tags, and entities, populated through thin adapters over the existing SentenceDetector, Tokenizer, POSTagger, and TokenNameFinder interfaces, which stay the primary API for single-task use and are unchanged. All spans refer to the text as supplied. No new dependencies.
…ntainer, javadoc precision pass
…rom missing layers, validate providers at build time
… adaptive data on failure The lemmatizer adapter now slices tokens and tags per sentence like its POS and name-finder siblings, so lemmatization decisions never cross a sentence boundary, and it declares the sentence layer as required. The POS adapter rejects a tagger that returns a wrong tag count. The name-finder adapter rejects mentions whose token indices lie outside their sentence instead of silently reading the next sentence's tokens, clears adaptive data even when annotation fails, and derives UNTYPED from NameSample.DEFAULT_TYPE instead of re-declaring the literal.
… definition A blank check under the toolkit's whitespace definition, which unlike String.isBlank covers the no-break spaces, so annotators validating labels and identifiers share one predicate instead of each carrying a private copy. Reads whole code points; tests pin the no-break and figure spaces, the empty string, and a supplementary-plane letter.
…nt rule Adds the Document Annotation Container chapter to the manual, with every code example and every stated span and value mirroring the passing pipeline example test. The review pass aligns the branch with the project's conventions: layer key ids validate through StringUtil.isBlank, the annotator interface leaves thread safety implementation specific, the sentence and tokenizer adapters document annotate like their siblings, repeated rejection-message literals become per-class constants, and the name finder test's nine anonymous fixtures fold into one helper. Layers now states the key placement rule: core layer keys live there, capability layer keys on their providing annotator.
Every key the toolkit defines now carries the opennlp: id prefix (opennlp:sentences, opennlp:tokens, opennlp:pos, opennlp:entities, opennlp:lemmas, opennlp:stems). An extension defines its keys under its own prefix, and a bare id stays legal for an application-local layer, so ids from independent producers cannot collide. The rule is stated on Layers, LayerKey, and in the manual chapter.
A layer key now declares whether its layer is positional or document-scoped. A positional key, the default, guarantees a span on every annotation, so consumers never null-check one. A document-scoped key, created through LayerKey.document, carries whole-document values without spans, the home for a language id, a category distribution, or provenance. The scope is declared per key, never per annotation: the container rejects a span-less annotation under a positional key and a spanned annotation under a document-scoped key, naming the layer either way. Scope participates in key equality.
…on text The three invariants the contract tests already enforce are now stated on the Document interface and in the manual chapter: layers preserve insertion order and are never reordered, layers are immutable once added and detached from the caller's input list, and adding a layer is once-only with the rejection naming the key. Together they keep index-based references between layers valid for the lifetime of the document.
A corpus may carry a hand-annotated version of a layer beside a produced one. The convention is a gold: id prefix on the same key scheme, for example gold:opennlp:tokens beside opennlp:tokens. Because adding a layer is once-only, competing versions of a layer always live under distinct keys and never replace each other. Stated on Layers and in the manual chapter, with a contract test pinning the coexistence.
…ctories on Layers
…le test
Add {@inheritdoc} to the Document, LayerKey, and adapter overrides, and note in
the manual that DocumentPipelineExampleTest asserts the pipeline round-trip.
…ainer contract - Reject zero-length finder mentions in NameFinderAnnotator and pin the second-sentence case, which was previously mapped silently wrong, with a test - Add DocumentAnnotators with requireLayers and the per-sentence token walk, replacing three copies of the walk loop and four spellings of the missing-layer rejection; direct tests pin the helpers as public API - Capture the document text as a String at construction so ImmutableDocument's immutability and thread-safety claims hold for mutable CharSequence inputs - Move the copy-on-add and threading narrative from the Document interface Javadoc to ImmutableDocument; the interface now states that thread safety is implementation specific - Carry the entity type as the annotation value only; entity spans are untyped, and the Javadoc names the value as the single source of the type - Make all six adapter annotators final before the types freeze - Align the TokenLengthAnnotator example with the documented required-layer contract in both the manual and the example test via requireLayers - Housekeeping per review: docbook CDATA placement, imports over qualified names, a ParameterizedTest for the blank-input matrix, shared deterministic test components, static assertion imports, inheritDoc on the runtime adapters, Layers constructor comment, and the documented NPE of StringUtil.isBlank
…ll rejection - Fold the three verbatim copies of the "Ana runs. Bob sits." document into a single twoSentenceDocument() helper in NameFinderAnnotatorTest, so the sentence and token layers of the shared fixture are declared once instead of drifting between the over-long mention, zero-length mention, and per-sentence offset tests - Hoist the no-op TokenNameFinder out of the blank-input test into a NO_NAMES constant in DocumentAnalyzerTest, since a finder that returns no spans is pipeline plumbing rather than part of any one test case, and document what it is for - Add testAnnotatorAdaptersRejectNullDocuments to pin that all four adapters reject a null document with the same "document must not be null" message, whether they validate directly or through DocumentAnnotators.requireLayers; the shared message was previously unpinned and free to drift per adapter - Trim the stale "person-free" qualifier from the New York comment in testTokenIndexSpansBecomeCharacterSpans; the finder emits a location mention and the extra negation described a distinction the test no longer draws
5ba76d6 to
e906225
Compare
…, pin blank and span edge cases - ImmutableDocument: wrap the layer map unmodifiable at construction and expose its cached key set; split the combined null check so the message names the offending argument - StringUtil.isBlank javadoc: state how it differs from isUnicodeBlank - Tests: parameterize the isBlank accept and reject sides, pin the null NPE, and pin char-indexed spans over a supplementary-plane character
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Left a few docs regarding the documentation and one suggestion.
| <para> | ||
| The design follows three rules: | ||
| </para> | ||
| <itemizedlist> |
There was a problem hiding this comment.
Can we make this less LLM generated? The bolt stuff at the beginning and the rest reads very "bla" ;-)
| highlighted in the source text. | ||
| </para> | ||
| <para> | ||
| The design follows three rules: |
| <title>Introduction</title> | ||
| <para> | ||
| The package <code>opennlp.tools.document</code> provides an immutable container | ||
| that carries the original text of one document together with any number of typed |
There was a problem hiding this comment.
what is a typed annotation layer? A user doesn't now that yet. maybe give an inline sxample.
| that carries the original text of one document together with any number of typed | ||
| annotation layers over it. Every annotation is anchored to a | ||
| <code>Span</code> of the text exactly as the caller supplied it, never to a | ||
| normalized or otherwise derived form, so any result of any pipeline step can be |
There was a problem hiding this comment.
A user doesn't now about pipeline steps yet. Can we rephrase it or link to a later explanation ?
| <para> | ||
| <emphasis role="bold">Offset-anchored.</emphasis> A layer is a list of | ||
| <code>Annotation</code> values, each pairing a <code>Span</code> in original | ||
| text coordinates with a typed value. Annotations reference other annotations |
There was a problem hiding this comment.
what is a original text coordinate?
| Adapters for the toolkit's own components are provided: | ||
| <code>SentenceDetectorAnnotator</code>, <code>TokenizerAnnotator</code>, | ||
| <code>POSTaggerAnnotator</code>, <code>NameFinderAnnotator</code>, | ||
| <code>LemmatizerAnnotator</code>, and <code>StemmerAnnotator</code>. Each wraps |
There was a problem hiding this comment.
Can we reword that? "single task" API or "primary API" bla bla? This doesnt read nice. Users can decide to use this new API for their single-task use ;-)
| .add(new TokenLengthAnnotator()) | ||
| .build(); | ||
|
|
||
| Document document = analyzer.analyze("The dog barks. It naps.");]]> |
There was a problem hiding this comment.
We could use exactly that example here to show a graphical version of a document, so we can reference it here later and people direcltryn ow what do expect?
| <section xml:id="tools.document.custom"> | ||
| <title>Writing a custom annotator</title> | ||
| <para> | ||
| A new capability contributes its results as one more layer without any change to |
There was a problem hiding this comment.
This is already known now, imho. First sentence can be dropped imho.
| }]]> | ||
| </programlisting> | ||
| <para> | ||
| Reading the layer back is statically typed by the key, so the values are used as |
There was a problem hiding this comment.
The "statically typed" bla is mentioned a lot of times in this doc. Please reduce.
| * | ||
| * @since 3.0.0 | ||
| */ | ||
| public interface Document { |
There was a problem hiding this comment.
what do you think about defining a Document merge(Document d) on the interface? Say you have processed the same document in parallel, so different layers are stacked in it now and you want to combine them (like adding the layers on top of the current one). wdyt?
There was a problem hiding this comment.
That's a great idea. We have the technology. We can build it. I'll work on it tonight after day job.
Addresses rzo1's review comments on the manual: - Open with a plain-language definition and an inline typed-layer example instead of a bolded three-item list. - Show a stacked-layers figure for the running example up front and reference it from the pipeline section, so readers see the shape of a document before the API detail. - Explain span offsets, key identity, and the opennlp: prefix convention in prose a first-time reader can follow. - Reword the single-task API aside; drop the redundant custom annotator opener; cut the repeated statically-typed phrasing.
Two contract tests fail red against the default method stub: java.lang.UnsupportedOperationException: merge is not implemented yet merge joins two documents grown independently over the same text, the parallel fan-out join rzo1 asked for on the pull request: disjoint layers stack into one document, the sources stay untouched, and a null argument, a different text, or a duplicate layer key is rejected with the offending key named.
The default body validates the argument and the shared text, then adds each of the other document's layers through with(), so every layer is re-validated against this document's contract and a duplicate key is rejected by the same once-only rule a direct add follows. The pinned contract tests now pass; opennlp-api is 386 tests, 0 failures.
Two contract tests fail red against the stubbed two-arg merge: java.lang.UnsupportedOperationException: merge with policy is not implemented yet The strict single-arg merge stays the default; the policy variant opts into keeping one copy of a layer both documents rebuilt identically, and still rejects differing copies with the key named.
merge(other) stays strict and now delegates to merge(other, REJECT). The KEEP_EQUAL policy keeps one copy of a layer both documents rebuilt identically, the shared sentence/tokenizer prefix of two parallel branches, while differing copies are still rejected with the key named. The pinned contract tests now pass; opennlp-api is 388 tests, 0 failures. The manual's fan-out paragraph documents the option.
- literallayout class=monospaced makes the docbkx toolchain emit a pre block, so the figure's character ruler and layer rows column-align; plain literallayout renders in the proportional body font. - Correct the pipeline section: the figure shows three of the four layers; the custom token-lengths layer is the fourth. - Trim restated clauses in the merge javadoc, the contract test javadoc, and the introduction; align the layersEqual and addLayer helper javadoc with what the helpers do.
The repinned test fails red: a KEEP_EQUAL merge that rejects a layer whose copies differ still reports 'layer is already present', which reads as if the policy was ignored. The caller opted into duplicates; the reason worth naming is that the contents differ.
…ayer When the policy is KEEP_EQUAL and a layer key is present on both documents, a failed equality check now throws directly instead of falling through to with(), so the message states the actual reason: the copies differ, not merely that the key is a duplicate. The pinned contract test passes; opennlp-api is 388 tests, 0 failures.
One javadoc sentence on the constant: equality is Annotation equality, so spans compare by offsets and type, never by probability, and values by their own equals. Two branches running different models over the same text can therefore agree; the kept copy is this document's.
Two guards ahead of an ImmutableDocument merge override: the interface default serves implementations that do not override merge with the same join, KEEP_EQUAL, and rejection messages, and merge re-validates the layers it takes from a foreign document instead of trusting them, rejecting an out-of-bounds span by name.
The interface default adds the other document's layers through with(), building one intermediate document and one map copy per layer. The override validates each incoming layer with the same checks with() runs, then copies the layer map once and allocates one document; when nothing was added it returns this, matching the default. The layer validation moves from with() into a shared helper unchanged. Pinned by the cross-implementation contract tests; opennlp-api is 390 tests, 0 failures, runtime annotator suites green.
The chapter said offsets count characters; the pinned contract test shows a supplementary-plane character counts as two. Say Java chars (UTF-16 units) so the claim matches the tested behavior.
Three tests fail red: the adapters inherit the identity toString, so a pipeline validation message reads 'annotator opennlp.tools.document.SentenceDetectorAnnotator@3b96c42e requires layer ...' instead of naming the adapter. The stemmer test pins the full analyzer message exactly, since that adapter requires a single layer and the message is therefore deterministic.
All six adapters override toString with the simple class name, so a pipeline validation message names the offending annotator readably. The pinned tests pass; opennlp-api and opennlp-runtime suites green.
2 participants
Adds the document annotation container discussed on OPENNLP-1888: an immutable
Documentover the original text, typedLayerKeyidentities, span-anchoredAnnotationvalues,DocumentAnnotatorwith declaredrequires/provides, and aDocumentAnalyzerwhose pipeline ordering is validated at build time. Four adapters over the existing single-task interfaces (sentence detector, tokenizer, POS tagger, name finder) plus lemmatizer and stemmer layer adapters come with it; the container itself never learns about specific layers.Contract behavior, each pinned by a test asserting the exact message where one is thrown: spans are structurally mandatory and validated against the text length; key equality is the (id, type) pair; layers preserve insertion order and are never sorted; layers are immutable once added and detached from caller input; providing an already-present layer is rejected; reading an absent layer returns an empty immutable list. 33 tests, including a full pipeline example and a contract suite.
Follow-ups planned as commits on this PR, from the review discussion on the ticket: namespaced identifiers for the standard keys; per-key positional versus document scope for whole-document facts; the invariants above transcribed into the specification text; and the documented convention for gold versus predicted layers. Opening as a draft until those land.
The acceptance criterion suggested in that discussion, that a new layer can be added without touching the container package, is already observable: the feature branches on the ai-pipestream fork (glossary, PII, coreference, dependencies, relations, money/quantity/temporal, geo, embeddings) each add their layers with no container edits.