[
https://issues.apache.org/jira/browse/OPENNLP-1904?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Martin Wiesner resolved OPENNLP-1904.
-------------------------------------
Resolution: Fixed
> Default-locale case folding corrupts lemmatizer models and output
> -----------------------------------------------------------------
>
> Key: OPENNLP-1904
> URL: https://issues.apache.org/jira/browse/OPENNLP-1904
> Project: OpenNLP
> Issue Type: Bug
> Affects Versions: 3.0.0-M5
> Reporter: Kristian Rickert
> Assignee: Kristian Rickert
> Priority: Minor
> Fix For: 3.0.0-M6
>
> Time Spent: 1h
> Remaining Estimate: 0h
>
> h2. Summary
> Several code paths fold case with the JVM default locale. In the lemmatizer
> this is
> not cosmetic: the folded strings become outcome labels that are *written into
> the
> trained model*, and the inference path folds again with whatever locale the
> consumer
> happens to run under. A model trained on a Turkish JVM therefore carries
> labels no
> other machine reproduces, and an existing English-trained model emits
> corrupted
> lemmas when served on one. Nothing logs, nothing throws; the output is simply
> wrong.
> h2. Reproduction
> Each of these is an actual assertion failure observed against unfixed code,
> under
> {{Locale.setDefault}} of {{tr}} (or {{de_DE}} for the report cases).
> {noformat}
> LemmatizerME.decodeLemmas expected <mouse> but was <mıuse>
> StringUtil.getShortestEditScript expected <D0s> but was <R6ıiD0s>
> LemmatizerME.encodeLemmas expected <R2ioR1cuI1s> but was
> <R2ıoR1cuI1s>
> DictionaryLemmatizer expected <Illinois> but was <O>
> TrainingParameters.setParams expected <50> but was <100>
> {noformat}
> The Turkish dotless i is the mechanism: {{"Illinois".toLowerCase()}} under
> {{tr}}
> yields {{{}ıllinois{}}}, which never matches an ASCII-lowercase dictionary
> entry, so
> {{DictionaryLemmatizer}} silently degrades to its {{"O"}} sentinel.
> h2. Affected sites
> ||Severity||Location||Expression||
> |Critical|{{opennlp-api/.../util/StringUtil.java:488}}|{{wordForm.toLowerCase()}}
> in {{getShortestEditScript}}|
> |Critical|{{opennlp-api/.../util/StringUtil.java:489}}|{{{}lemma.toLowerCase(){}}},
> same method|
> |Critical|{{opennlp-core/opennlp-runtime/.../lemmatizer/LemmatizerME.java:171}}|{{toks[i].toLowerCase()}}
> in {{decodeLemmas}}|
> |Critical|{{opennlp-core/opennlp-runtime/.../lemmatizer/DictionaryLemmatizer.java:164}}|{{word.toLowerCase()}}
> in {{getDictKeys}}|
> |Moderate|{{opennlp-ml-commons/.../util/TrainingParameters.java:332-337}}|{{{}ITERATIONS_PARAM.toLowerCase(){}}},
> {{CUTOFF_PARAM.toLowerCase()}}|
> |Moderate|{{opennlp-ml-commons/.../ml/model/AbstractModel.java:34}}|{{static
> final DecimalFormat}} without locale, used by {{getAllOutcomes()}}|
> |Moderate|{{opennlp-core/opennlp-cli/.../cmdline/DetailedFMeasureListener.java:119}}|{{createReport(Locale.getDefault())}}|
> |Moderate|{{opennlp-core/opennlp-cli/.../cmdline/FineGrainedReportListener.java}}|9
> {{MessageFormat.format}} sites|
> Sites 1 to 3 are one defect in two halves. {{getShortestEditScript}} produces
> the
> labels via {{LemmatizerME.encodeLemmas}} and {{{}LemmaSampleEventStream{}}};
> {{decodeLemmas}}
> consumes them. They must change together or train/serve skew is reintroduced.
> h2. Why {{Locale.ROOT}} and not {{StringUtil.toLowerCase}}
> This is the load-bearing decision in the patch and a reviewer will likely
> raise it.
> The project's own {{StringUtil.toLowerCase}} maps code points through
> {{{}Character.toLowerCase{}}}. That is context-free, and it *disagrees* with
> {{String.toLowerCase(Locale.ROOT)}} on context-sensitive mappings (Greek
> final sigma:
> {{ΟΔΟΣ}} folds to {{οδοσ}} rather than {{{}οδος{}}}) and on one-to-many
> mappings such as
> {{{}ß{}}}.
> Every shipped model and dictionary was produced on an English-locale JVM,
> whose fold
> is byte-identical to {{{}Locale.ROOT{}}}. So {{Locale.ROOT}} keeps those
> artifacts working
> unchanged, while the code-point fold would silently alter behaviour for every
> existing
> model.
> *No model regeneration is required.* That is the point of the choice. A guard
> test
> ({{{}StringUtilLocaleTest.testGetShortestEditScriptUsesRootFoldingNotCodePointFolding{}}})
> exists specifically to fail if someone later "simplifies" this to
> {{StringUtil.toLowerCase}} for consistency.
> h2. Deliberately not changed
> Reporting these would waste review time; all were checked and are already
> safe:
> {{{}StringUtil.toLowerCase{}}}/{{{}toUpperCase{}}} (code-point based),
> {{{}String.CASE_INSENSITIVE_ORDER{}}},
> all {{equalsIgnoreCase}} uses, {{String.format("U+%04X")}} (the
> {{{}x{}}}/{{{}X{}}}/{{{}o{}}}
> conversions are never localized per the {{Formatter}} spec),
> {{{}NameFinderDL{}}}'s own
> fold, and all model serialization ({{{}Double.toString{}}},
> {{{}DataOutputStream{}}}).
> Two residual {{toUpperCase()}} calls in main sources are benign and were left
> alone:
> {{EmojiFlags.java:171}} (hex digits in an exception message) and
> {{GenerateManualTool.java:227}} (DocBook titles in a build-time doc
> generator).
> h2. Behaviour change to note in release notes
> {{DetailedFMeasureListener.createReport()}} and {{FineGrainedReportListener}}
> now
> always render with {{{}Locale.ROOT{}}}. Eval report output is therefore no
> longer
> localized. This was chosen so reports are reproducible, diffable and
> machine-parseable
> across machines. The explicit {{createReport(Locale)}} overload is untouched,
> so a
> caller who wants localized output can still ask for it.
> This is the one judgement call in the patch and deserves a maintainer sanity
> check.
> h2. Test coverage
> There was *no locale test coverage anywhere in the tree* before this branch.
> There is
> no {{Locale.setDefault}} in main or test sources; the only Turkish-aware
> tests are
> Snowball stemmer data tests, which exercise Turkish {*}input{*}, not a
> Turkish {*}JVM locale{*}.
> Nine new tests, each verified to fail before the fix and pass after (the fix
> was
> stashed, the build rerun, the failure observed, then restored). Failure
> messages are
> in the Reproduction section above.
> Two gaps in that coverage:
> * {{AbstractModel.DECIMAL_FORMAT}} is fixed *without* a red/green test. The
> formatter is
> a {{static final}} resolved at class initialization, so any
> {{Locale.setDefault}} in a
> test runs after the class is loaded and such a test passes both before and
> after. A
> fresh-classloader harness would need to reload an abstract class plus
> {{Context}} and
> {{EvalParameters}} and reflectively build a concrete subclass; adding
> {{-Duser.language=tr}} to the surefire {{argLine}} would flip the locale for
> every
> other test in the module. Neither seemed worth it for a formatter used only
> by the
> diagnostic {{{}getAllOutcomes(){}}}.
> *
> {{StringUtilLocaleTest.testGetShortestEditScriptUsesRootFoldingNotCodePointFolding}}
> is a {*}guard{*}, not a regression test. It passes before and after, because
> under an
> English default the two folds agree. Its purpose is to break on a future
> "simplification". Flagged because it does not meet the fails-before bar.
> One drafted test was discarded after it passed unfixed: an encode/decode
> round trip is
> self-consistent within a single locale and so could never fail. It is not in
> the commits.
> Note for anyone adding more: the root {{pom.xml}} surefire \{{argLine}}s do
> *not*
> forward {{{}-Duser.language{}}}, so {{mvn -Duser.language=tr test}} does not
> reach the
> forked test JVM.
> h2. Build
> {{mvn -o clean install}} over the full 23-module reactor: BUILD SUCCESS, zero
> failures
> or errors, with checkstyle, rat and the existing forbiddenapis configuration
> active.
> {{mvn javadoc:javadoc}} clean on the five touched modules (three pre-existing
> unknown-tag warnings, none on a changed line).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
