Repository navigation
Merge equivalent MSAs when loading HermitCrab - #1148
Conversation
There was a problem hiding this comment.
Keeping the first of several equal MSAs changes which MSA the parser reports. Glosses are mostly fine: SenseWithMsa returns only the first sense that uses an MSA, so a project whose senses correctly share one MSA also shows one gloss per parse, and deduplicating makes duplicate data behave like clean data there.
What breaks is identity. Saved analyses and ad-hoc prohibitions point at a specific MSA. After the loader drops a duplicate, analyses using it stop matching (ParseResult.MatchesIWfiAnalysis compares MsaRA by identity) and get a parser "disapproves" from SetUnsuccessfulParseEvals, and prohibitions naming it are discarded in LoadMorphemeCoOccurrenceRules.
Suggestion: fix the data, not the loader. liblcm already merges EqualsMsa-equal MSAs when entries are merged (LexEntry.MergeObject, OverridesLing_Lex.cs ~3524, LT-13007), and MergeObject redirects every reference to the dropped MSA and combines reference collections such as slots.
- Now: a Tools > Utilities fixer, next to
DuplicateAnalysisFixer, that does that merge within each entry. Senses, analyses and prohibitions all follow the kept MSA, and the duplicate parses go away. - With the upcoming liblcm model bump: the same merge as a one-time data migration, so every project is repaired once (draft Jira issue below).
If the loader deduplication stays until projects are repaired, it would need:
- Dropped MSAs registered in
m_morphemesunder the kept entry, so prohibitions naming them still apply. - Parse filing that matches saved analyses by group, not exact
MsaRA, with the dropped-to-kept map passed from the loader. - A predictable survivor: the MSA of the first sense, which gives the same gloss a clean project would show.
HCLoaderTestsfor the grouping, variants and prohibitions, plus aParseFilertest for a saved analysis on a dropped MSA.- A scope decision on affix MSAs, where the Aweti reduplication duplicates live.
For the Aweti speed problem, sillsdev/machine#519 takes the worst word from out-of-memory to 9 s by pruning reduplication splits soundly, without touching MSAs.
Draft Jira issue (not filed yet)
- Type: Task, project LT, Affects Version FW 9.3
- Summary: Data migration: merge equivalent MSAs within each lexical entry
- What this is: a one-time liblcm data migration that merges
EqualsMsa-equal MSAs within each entry, using the existingMergeObjectpath. - Who it affects: projects with duplicate grammatical info on an entry. The parser builds one entry per duplicate, so words get duplicate parses.
- Why it matters: it removes the duplicate parses at the source, while senses, saved analyses and ad-hoc prohibitions keep pointing at a valid MSA.
- Links: LT-22615 (related: creating a distinct MSA on purpose), LT-13007 (precedent), this PR.
|
@johnml1135 A possible disadvantage of changing the data is that external data (like interlinear texts) may become invalid. We should check with Jason before pursuing this approach. Merging entries is not a good analogy, since one of the entries disappears in any case. If we merge MSAs, then all of the senses are still there. |
NUnit Tests 1 files ± 0 1 suites ±0 13m 41s ⏱️ + 1m 18s Results for commit d200abc. ± Comparison against base commit 2f1d9db. This pull request removes 2 and adds 170 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1148 +/- ##
==========================================
+ Coverage 38.89% 39.31% +0.41%
==========================================
Files 1518 1523 +5
Lines 352262 353117 +855
Branches 40597 40761 +164
==========================================
+ Hits 137005 138818 +1813
+ Misses 185976 185029 -947
+ Partials 29281 29270 -11
🚀 New features to boost your workflow:
|
johnml1135
left a comment
johnml1135
left a comment
There was a problem hiding this comment.
A few line-level follow-ups on the current head (edded8e). Thanks for mapping RestOfMorphsRS and the saved-analysis matching since my earlier review; the comments below are the places that still see a merged-away MSA directly.
Suggested title: "Merge equivalent MSAs within an entry when loading HermitCrab". The PR now covers affix MSAs too and adds a MergeMSAs option, so "Add UniqueStemMSAs" undersells it. The description still describes keep-first, stems only and no test; it would help to bring it up to date.
| } | ||
|
|
||
| [Test] | ||
| public void RepresentativeMSAs() |
There was a problem hiding this comment.
This covers affix merging. Proposed additional cases:
Stem MSAs merge
- Given an entry with two senses whose stem MSAs are
EqualsMsa-equal - When the lexicon is loaded with merging on
- Then there is one HC lexical entry, with the first sense's gloss
Survivor is a referenced MSA (the Aweti kãᴷ shape)
- Given stem MSA A that no sense uses and an equal MSA B used by a sense
- When loaded
- Then the one entry carries B's hvo and B's sense gloss (not
???)
Variants
- Given a variant of a main entry that has two equal stem MSAs
- When loaded
- Then one variant entry is created
Different MSAs stay separate
- Given two stem MSAs with different POS in one entry, and equal MSAs in two different entries
- When loaded
- Then all of them load as separate entries
Ad-hoc prohibition on a merged-away MSA
- Given a morpheme prohibition whose
FirstMorphemeRA(and, separately, one of whoseRestOfMorphsRS) is a merged-away MSA - When loaded
- Then the co-occurrence rule is still attached to the kept morpheme
Saved analysis on a merged-away MSA
- Given a
WfiAnalysiswhose morph bundle uses merged-away MSA B - When the parser returns the same parse with representative MSA A
- Then
MatchesIWfiAnalysisis true andParseFilerneither adds a duplicate analysis nor disapproves the saved one
Option off
- Given
MergeMSAs= false - When loaded
- Then duplicate MSAs load separately and matching is by exact MSA
There was a problem hiding this comment.
I added "Option off". I think that the unit test has coverage that is comparable to other features in HCLoaderTests, but you can add more tests if you want.
There was a problem hiding this comment.
Thanks for the option-off test. The one case I'd still like covered is the main path: two senses on EqualsMsa-equal stem MSAs load as one HC entry. Happy for that to be a follow-up instead of blocking this PR. Want me to file an LT issue for it?
jtmaxwell3
changed the title
jtmaxwell3
changed the title
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
Reviewed a bit as well as looking at the question you raised for me. It looks like a good change, and the survivor rule (a referenced MSA wins) is the right call. One fix inline for a rare data shape that makes the load throw, one question, and my answer on the approach question is in the conversation.
This review was assisted by Claude Fable 5.1.
| ILexEntry entry = msa.OwnerOfClass<ILexEntry>(); | ||
| foreach (var msa2 in entry.MorphoSyntaxAnalysesOC) | ||
| { | ||
| if (msa2.EqualsMsa(msa) && entry.SenseWithMsa(msa2) != null) |
There was a problem hiding this comment.
Fix needed, for rare data: MoDerivStepMsa.EqualsMsa is throw new NotImplementedException(...) in liblcm (OverridesLing_MoClasses.cs, and the shipped SIL.LCModel.dll carries it), and it throws before any type check. This loop calls msa2.EqualsMsa(msa) on every MSA in the entry, so one DerivStep MSA anywhere in the entry, as msa2 or as msa, throws out of LoadParser, which nothing catches. Today the loader ignores that class because LoadMorphologicalRule has no case for it. Only old or exceptional data has one: FLEx's MSA creator cannot make it, and it arrives via LIFT import of a derivStepAffix trait. Suggest returning msa when it is an IMoDerivStepMsa, and comparing only when msa2.ClassID == msa.ClassID.
| using System.IO; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Web.Caching; |
There was a problem hiding this comment.
Question: is System.Web.Caching meant to be here? Nothing in the file uses it (the Cache references are the LcmCache field), and ParserCore.csproj lists no System.Web reference, so it compiles only because the net48 reference set happens to include it. Looks like an IDE auto-import.
|
I think that we should proceed with this change. It is safe and effective. I think we can do the data change but I would want to make a thorough plan with full impact analysis. This could be part of a data migration. I think we should track this idea in a jira issue marked as data migration for us to work on later. |
johnml1135
left a comment
johnml1135
left a comment
There was a problem hiding this comment.
Both of Jason's points look fixed in 73c25ef: DerivStep MSAs now skip EqualsMsa, and the System.Web.Caching import is gone. From my side, this is just waiting on Jason's review.
This change is visible to users: there's a new MergeMSAs parser parameter, and Try A Word now shows one parse per merged MSA. Should I file an LT ticket so testers check it in the nightly?
If you can set the parameter through the UI I think that would be a good idea. File it and get it into the sprint with an acceptance test. |
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
Both fixed, thanks. LGTM.
This review was assisted by Claude Fable 5.1.
|
I'm planning to include tests for this as part of https://jira.sil.org/browse/LT-22780 (Improve Hermit Crab performance). |
4 participants
Start here:
UniqueStemMSAsinSrc/LexText/ParserCore/HCLoader.cs, then its two call sites inLoadLexEntries.What it does. When
HCLoadertranslates a FieldWorks lexicon to HermitCrab, it builds one HermitCrab entry per MSA. Users often create a new, identical MSA when adding a sense instead of reusing the existing one, so an entry with three identical "Noun" MSAs becomes three HermitCrab entries. They have the same allomorphs, category and features, and differ only in gloss. Every word built on that stem then parses three times, and the parser does three times the work for it. This PR keeps only the first glossed MSA in each group thatEqualsMsacalls equal (category, inflection class, features, from-categories, production restrictions), for ordinary entries and for variants of a main entry.The unknown you start with: what changes for users? Parses of affected entries are no longer duplicated, and the surviving parse carries the kept MSA's gloss. Equivalent MSAs in different entries are never merged.
Where to look
kãᴷlists an unreferenced MSA first; its one sense (to.dry) points at the second. After this PR its only entry carries a null gloss, andHCParsershows the placeholder glossksQuestions. Preferring an MSA thatSenseWithMsaresolves would avoid this.LoadMorphologicalRulesstill builds one rule per affix MSA. The Aweti reduplication entryllhas 9 derivational MSAs, 6 of them redundant copies of an earlier one. They still become 9 rules, so the Aweti reduplication case this change was written for is not fixed here.HCLoaderTestshas no duplicate-MSA case.Downstream. PanGloss (a Rust port of HermitCrab) mirrors this loader and has a matching port waiting on this PR: sillsdev/PanGloss#3.
Verification. Not built or run. The duplicate counts come from a script over the project
.fwdatafiles (see Evidence).Next: decide between keep-first and prefer-referenced, and whether affix MSAs belong in this PR or a follow-up; then add an
HCLoaderTestscase.Evidence: duplicate MSAs in five real projects
Counted directly from the
.fwdataXML: an MSA counts as a duplicate when its serialized content, with GUIDs removed and owned objects inlined, equals an earlier MSA of the same class in the same entry. That is stricter thanEqualsMsa(ordered, exact), so these counts are lower bounds.kãᴷ)ll)amyali)In Sena's
amyalithe referenced MSA comes first, so keep-first happens to keep the right one there. In Aweti'skãᴷit does not.Original description
This makes stem MSAs unique within a lexicon entry when translating from FieldWorks to HermitCrab. MSAs are defined locally to a lexicon entry, and are supposed to be unique, but the user sometimes duplicates MSAs when defining new senses rather than reusing existing ones. Having duplicate MSAs affects the performance of Hermit Crab, since it produces duplicate rules if the senses only differ semantically, not syntactically. This not only introduces spurious ambiguity, but allows the rule to be applied multiple times (otherwise, rules can only be applied once). This is particularly bad for the Aweti grammar, which has multiple versions of the reduplication rules that only differ semantically.
How the gloss is lost
LoadLexEntrysetshcEntry.Gloss = GetGloss(msa)andProperties[MsaID] = msa.Hvo.GetGlossreturnsSenseWithMsa(msa)?.Gloss, which is null for an MSA no sense references. At parse timeHCParserresolves the MSA fromMsaIDand again callsSenseWithMsa; a null sense yieldsParserCoreStrings.ksQuestionsas the gloss.This change is