Conversation
rzo1
left a comment
There was a problem hiding this comment.
Two structural changes needed before this can go in.
1. No C/C++ sources in the repo
Drop dev/hunspell-reference.cc. Maven never compiles it, CI never runs it, and no toolchain in this project covers it, so it will go stale without anyone noticing. It also exists only to link against an external GPL/LGPL/MPL project.
The method is worth documenting. Keep dev/README-hunspell-dictionaries.md and describe how you produced the reference output: the pinned Hunspell revision e184e22c..., the C API functions you called, and the g++ command. Anyone who wants to reproduce it can write those 30 lines themselves.
That means HunspellTestSupport in opennlp-runtime also has to lose the ProcessBuilder call and the opennlp.hunspell.reference property. Unit tests in opennlp-core must not fork an external native binary. The fixture dictionaries and the expected outputs you derived from the reference are the right thing to commit.
2. Eval tests should follow the existing pattern
HunspellCompatibilityEval defines its own contract: three custom system properties (opennlp.hunspell.dict.dir, opennlp.hunspell.eval.words.dir, opennlp.hunspell.reference), Assumptions-based skipping, and required input files that live nowhere we control.
The other evals in that module extend AbstractEvalTest and resolve inputs through getOpennlpDataDir() (-DOPENNLP_DATA_DIR=... -Peval-tests), checking data with verifyFileChecksum / verifyDirectoryChecksum. Do the same here:
class HunspellCompatibilityEval extends AbstractEvalTest- Read dictionaries from
new File(getOpennlpDataDir(), "hunspell/...")instead of a custom property. - Keep the SHA-256 pins, expressed the way the module already does it.
- Remove the
Assumptionsskips. Evals run under-Peval-testsagainst data that is expected to be there, and fail if it isn't.
The LibreOffice dictionaries and word lists go into opennlp-data.zip on nightlies.apache.org/opennlp/, under a hunspell/ subdirectory, not into a directory each contributor assembles by hand. Any committer can update that archive:
curl -u your_asf_username -T ./opennlp-data.zip "https://nightlies.apache.org/opennlp/"Note that the checksum file published next to the zip has to be regenerated and uploaded in the same step, otherwise the eval job fails against the new archive. See https://nightlies.apache.org/authoring.html. If you don't have an ASF account, list the exact files and revisions and one of us will upload them. Nothing is downloaded at build time either way.
Revert the addition to opennlp-docs/src/docbkx/evaltest.xml. Once the eval uses OPENNLP_DATA_DIR, that chapter already covers it. Hunspell specifics belong in stemmer.xml.
One more thing: HunspellRealDictionaryTest is removed (126 lines) without a note in the description. What covers that now?
|
There was a lot I wasn't sure about which is why I kept it in draft, thanks for chiming in. The driver source and the test support that forked it are removed. The README describes how the reference results were produced: Hunspell e184e22c, the C API calls, the input and output convention, and the build line. The fixture tests keep the captured reference outcomes and assert OpenNLP results against them; each intended deviation is named with its reason from the manual, and a named fixture must still differ, so the data cannot go stale silently. HunspellCompatibilityEval now extends AbstractEvalTest, loads from hunspell/ under OPENNLP_DATA_DIR, verifies MD5 digests with verifyFileChecksum, and has no Assumptions or custom For opennlp-data.zip, under hunspell/, from LibreOffice dictionaries commit 32b006a2c22a4ac7e8ed3f03346f7b3d85a970a4: en_US.aff, en_US.dic, README_en_US.txt, de_DE_frami.aff, de_DE_frami.dic, README_de_DE_frami.txt, hu_HU.aff, hu_HU.dic, README_hu_HU.txt. The MD5 pins in the eval match these files. HunspellRealDictionaryTest was a property-gated test in opennlp-runtime against the same three dictionaries. Its stem and compound assertions moved verbatim into HunspellCompatibilityEval.expectedInflections, which is where external data belongs; I will add that to the description. I feel like I have to do one more pass then flip to ready. |
|
@rzo1 I'll give the upload a shot :) I have the zip, a SHA, and a decent connection speed - should work for me. I want to do this because I'll probably have to start adding data to this more than once. If it doesn't work I'll let ya know. |
|
We will target this PR / topic with the 3.0.0 release, not M6. |
|
Turning to draft, I'm adding tests and doing a few minor changes. |
d6e762b to
fbdc1f3
Compare
rzo1
left a comment
There was a problem hiding this comment.
Thanks, the points from my last review are addressed: no native sources, no forked binary in unit tests, the eval follows the AbstractEvalTest pattern, and the data is in opennlp-data.zip.
Still needed before merge:
- Fixture provenance.
HunspellCompatibilityTestsays the fixtures are independently written and not excerpts. Several are taken from Hunspell'stests/ate184e22c, e.g.compoundforbid.aff/.dicandgh106.aff/.dicare identical, andcheckcompoundpattern2,onlyincompound2,ph2,opentaal_forbiddenword1andallcapsare close.HunspellDictionaryeven linkstests/compoundforbid.aff. Hunspell is MPL/GPL/LGPL. These snippets are small, but I don't want to guess. Please write our own fixtures (own words and flags, same rule being tested), or name the source of each one so we can decide. In both cases the Javadoc claim has to go. - Behavior change since M6. In 3.0.0-M6
load(...)ignored unsupported directives. Now it defaults toSTRICTand throwsIOException, so dictionaries that loaded in M6 can fail now. That's fine for 3.0.0, but it needs a line in the JIRA as release note. HunspellStemmer#analyze. Public, returns raw Hunspell field strings, and only tests and the eval use it. This is frozen with 3.0.0. Either make it package-private for now, or explain in the JIRA why this is the API we want.- Allocations in the stemming path. See the inline comments.
Also: HunspellDictionary#parseAffix is 175 lines. Please split it per directive group.
Please also post the eval build link for the current head.
|
rzo1 - this rung's for you. All points are addressed on 8539b45 (merged with main today); eval run: https://ci-builds.apache.org/job/OpenNLP/job/eval-tests-configurable/84/. AI summary:
|
Add dictionary directives, stemming and compound handling with explicit strict/partial loading, focused fixtures, documentation and evaluations. Include the reviewed fixture, API and allocation fixes. Squash the reviewed public history without changing its final tree. Prior exact-tree validation: hosted Java CI passed and Jenkins evaluation 84 succeeded on 8539b45.
8539b45 to
3510fe6
Compare
Extends Hunspell dictionary loading, stemming, and morphological analysis. Unsupported directives fail by default; partial loading is an explicit choice that reports the skipped directives.
The engine now covers the directives the ticket names and the further behavior a sweep of the reference implementation's own test fixtures turned up: zero-material affixes, compound restrictions, forbidden-word ordering, hidden capitalized forms, Turkic and mixed-case input, the Hungarian hyphen rule, and the reference field order of analyses. The manual lists the remaining deviations with a workaround for each.
The fixture tests keep the stems and recognition outcomes captured from Hunspell
e184e22con fixtures written for the tests; analyses are compared inHunspellCompletionTestand assert OpenNLP results against them; no native code or process is part of the build.HunspellCompatibilityEvalextendsAbstractEvalTest, loads the LibreOfficeen_US,de_DE_frami, andhu_HUdictionaries fromhunspell/underOPENNLP_DATA_DIRwith MD5 checks, and compares the stems and recognition of 49 inputs with the captured Hunspell results. The dictionaries are inopennlp-data.zipon nightlies as of 2026-09-07.HunspellRealDictionaryTest, a property-gated test in opennlp-runtime, is removed; its assertions moved into that eval.No dictionaries or native libraries are bundled. The shared
Stemmerinterface is unchanged.HunspellStemmer.analyzeis package-private; the public API returns stems. Loading defaults toSTRICT, a change since M6 noted in the JIRA release note.Verification: opennlp-runtime 3,134 tests with checkstyle; the eval, 13 tests, against the local data directory and the published archive.
OPENNLP-1927