Conversation
…C helper base # Conflicts: # opennlp-docs/src/docbkx/dependency.xml
…C helper base # Conflicts: # opennlp-docs/src/docbkx/dependency.xml
|
@krickert Can you rebase this one onto main? Currently changes show up here that are now already included in main. |
71da201 to
83bc298
Compare
# Conflicts: # opennlp-docs/src/docbkx/dependency.xml
83bc298 to
5cc4058
Compare
|
Rebased on main .. good to go #1182 merged the diff is the parser alone, 34 new files and no change to existing code. Also a run of the review conventions. |
# Conflicts: # opennlp-api/src/main/java/opennlp/tools/depparse/DependencyGraph.java # opennlp-api/src/main/java/opennlp/tools/depparse/DependencyParser.java # opennlp-api/src/main/java/opennlp/tools/depparse/DependencySample.java # opennlp-api/src/test/java/opennlp/tools/depparse/DependencyGraphTest.java # opennlp-api/src/test/java/opennlp/tools/depparse/DependencySampleTest.java # opennlp-core/opennlp-formats/dev/README-ud-treebanks.md # opennlp-core/opennlp-formats/dev/download-ud-treebank.sh # opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/conllu/ConlluDependencySampleStream.java # opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/conllu/ConlluDependencyParserEvalTest.java # opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/conllu/ConlluDependencyParserUsageTest.java # opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/conllu/ConlluDependencySampleStreamTest.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/ArcStandardOracle.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/ArcStandardState.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/DependencyContextGenerator.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/DependencyModel.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/DependencyParserME.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/FeedforwardContext.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/FeedforwardDependencyModel.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/FeedforwardDependencyParser.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/FeedforwardDependencyTrainer.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/depparse/Transition.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/depparse/ArcStandardOracleTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/depparse/ArcStandardStateTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/depparse/DependencyParserEdgeCaseTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/depparse/DependencyParserMETest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/depparse/DependencyTestSamples.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/depparse/FeedforwardDependencyParserTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/depparse/TransitionTest.java # opennlp-docs/src/docbkx/dependency.xml
|
Thanks @krickert. A long open JIRA ticket for sure! The API and graph validation look solid. I found two issues worth checking into:
What do you think about held-out accuracy tests for the neural parser, similar to the treebank evaluation for the classical parser? |
|
Thanks Jeff. Both are fixed, each with a failing test committed ahead of the fix. The CoNLL-U reader now skips a sentence whose relation column is the underscore placeholder, the same way it already skipped an underscore head. The feedforward trainer no longer lets a tag or label spelled like a reserved symbol take over that symbol's embedding row; it shares the row, as words already did, so the model reloads. On held-out accuracy: I added |
|
Eval build for this head (afd68fb) is running on Jenkins: https://ci-builds.apache.org/job/OpenNLP/job/eval-tests-configurable/74/ (eval-tests-configurable, branch OPENNLP-547-dependency-parser). It uses the opennlp-data.zip published on nightlies today and includes the new UniversalDependencyParserEval, so expect it to take longer than a regular eval run. |
|
Evals passed for this, going to re-run it as I added double precision scoring. |
Moved to draft for now. Please re-open as ready for review once all your additional checks and stuff are done. |
|
https://ci-builds.apache.org/job/OpenNLP/job/eval-tests-configurable/76/console Eval build is running |
|
Eval ran successfully |
rzo1
left a comment
There was a problem hiding this comment.
Thanks. jzonthemtn's two issues are fixed with tests, and the eval pattern is right.
My main concern is the model. FeedforwardDependencyModel is not a BaseModel: it has its own binary format (ONLP-FFDP-1) with no manifest, language, version check or artifact serializer. Once released we have to support that format. The same goes for the rest of the component shape: training takes a Settings record instead of TrainingParameters, and there's no DependencyParserFactory extends BaseToolFactory, no CLI tools, and no stream factory registration. Every other component in OpenNLP works this way, and I don't want a second model format with 3.0.0.
I'd suggest splitting it:
- This PR: API (
DependencyParser,DependencySample,DependencyGraph), the CoNLL-U reader, evaluator and arc-standard parts, aligned with the other components (Sample,serialVersionUID,TrainingParameters, factory, CLI). - Follow-up: the feedforward model as a
BaseModel, with a new eval run.
If you'd rather keep it in one PR, the model format has to be settled before we look at the details.
Other points:
- OPENNLP-547 has no fix version. Please set
3.0.0. - There are 147 commits, some without a JIRA key. We will squash-merge.
- Duplication, see inline comments.
dev/download-ud-treebank.sh, its README andUdTreebankDownloadScriptTest: the build doesn't use the script, since the eval reads fromopennlp-data, and the unit test starts bash. Please remove all three and the reference independency.xml.
|
@krickert Can you fixed up this branch plz? It has 147 commits and I don't think they belong here. |
Provide the parser API, CoNLL-U reader, arc-standard model, evaluation, factory and CLI integration. Apply the component review fixes and move the neural implementation to OPENNLP-1991. Validation: affected reactor verify (4621 tests, six existing skips), five pinned treebank evaluations, Javadoc and HTML manual. Regression tests reproduced the Sample contract and missing-POS failures before the fixes.
5b05ca9 to
e2d7be8
Compare
|
R4R @rzo1 |
Summary
Base
Rebased on main. The Document annotation container (#1182) is merged, so the branch adds only the parser: 34 files, all new, with no change to existing code.
Review round of 2026-09-04
The branch carries the review conventions from the sibling PRs: Javadoc on every method including private helpers and constructors,
@throwson each validating path, validation at the public boundary withIllegalArgumentException, named constants, the final-sigma rule in vocabulary normalization, bounded model dimensions, and failing-test commits ahead of each fix.Verification
opennlp-api,opennlp-runtime, andopennlp-formatsbuild with checkstyle and forbidden APIs; the parser and CoNLL-U test classes pass (128 tests), and the manual chapter's examples are mirrored byConlluDependencyParserUsageTest.