Skip to content

Add bounded batch parsing to HermitCrab - #582

Open
johnml1135 wants to merge 1 commit into
masterfrom
feat/hc-parse-words-batch
Open

johnml1135 wants to merge 1 commit into
masterfrom
feat/hc-parse-words-batch

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Start here: src/SIL.Machine.Morphology.HermitCrab/Morpher.cs, ParseWords: the whole public surface, plus the guard that requires a degree-1 Morpher.

Status: ready for review; all checks green. #546 (shared template optimization, still open) is not required; this PR is independent of it.

FieldWorks consumer: FLEx's parser worker adopts this through ParseWords on a degree-1 Morpher, tracked in LT-22885.

Morpher.ParseWords(words, maxWordConcurrency = 4, guessRoot = false) parses a batch of words concurrently and returns one WordParseResult per input position, in input order, duplicates kept. Each result carries materialized analyses, a trace, and any per-word exception, so an invalid shape or resource-limit failure on one word does not discard the others. The single-word API and the constructor default (maxDegreeOfParallelism = 0) are unchanged.

What could break existing callers? Nothing opts in implicitly. ParseWords throws unless the Morpher was built with maxDegreeOfParallelism: 1, which keeps template slots, analysis cascades and synthesis sequential inside each word. The one shared-state change: alternative limits now belong to the invocation, removing writes to shared compiled rule objects during parsing (AnalysisStratumRule.cs, AnalysisLanguageRule.cs).

var morpher = new Morpher(traceManager, language, maxDegreeOfParallelism: 1);
IReadOnlyList<WordParseResult> results = morpher.ParseWords(pendingWords);

Where to look:

  • Per-invocation limits replacing the old _maxAlternatives field: AnalysisStratumRule.cs. Pinned by ParseWords_CapturesMaxAlternativesPerWord.
  • Results match single-word parsing, in order: ParseWords_PreservesInputOrderAndSingleWordAnalysisSets, ParseWords_MatchesSingleWordAnalysisSetsWithCompounding.
  • Degree-0 Morphers are refused: ParseWords_RequiresSequentialMorpher.
  • Tracing runs the whole batch serially on the calling thread: ParseWords_TracingRunsOnCallingThreadAndRetainsPerWordTraces.

Deliberately not here: FieldWorks adoption (it needs a caller change; the default stays 0); a change to the single-word constructor default; hard or capped words are unmeasured for parity and throughput.

Speed figures are measured wall timings used only as motivation, not a guarantee for this implementation or every Language (details below).

Verification: offline Release build passes with zero warnings or errors; the full HermitCrab NUnit project passes, including the new ParseWords_* cases; CSharpier checks the five changed files; comment hygiene is clean. Repository-wide ./local_check.sh and coverage were not run locally.

Next: review the ParseWords signature and the degree-1 requirement; say if you want the requirement relaxed.


Reading this a year from now -- start here

ParseWords exists so a caller with a queue of pending words can spend parallelism across words instead of inside one word. The measurements, FieldWorks adoption notes and the PanGloss comparison below are the reasoning behind the API shape; the tree only shows the API.

Evidence: measured speedups (wall time, secondary)

A local benchmark harness, not part of this PR, measured four concurrent degree-1 words at 3.05x on Sena and 2.20x on Mbugwe versus the degree-0 serial-word default. Those figures include the separate #546 shared template optimization. The same experiment with sharing disabled measured 2.42x and 1.47x.

Caveats: three-pass median wall timings on a shared machine, excluding construction, over small completed cohorts (62 Sena positions; 15 of 42 sampled Mbugwe positions). The harness used one Language/Morpher per worker, whereas this API shares one fixed Morpher. Hard or capped words remain unmeasured; the larger Sena run was unstable under its watchdog. Twenty concurrent words performed worse than four on both primary real-input cohorts, hence the default of 4.

Decisions and why
  • Degree-1 Morpher required. Bounded parallelism across words with sequential work inside each word. The evidence does not justify silently changing the existing single-word constructor default.
  • Differs from PanGloss on purpose. Its memory architecture differs, so the C# API puts parallel work across words and sequential paths within each word.
  • Tracing is serial. Traced calls sharing a trace manager must not overlap, so a traced batch runs on the calling thread.
Deferred: what FieldWorks adoption needs

FieldWorks's parser worker currently submits one word at a time and constructs Morpher without a degree argument (Src/LexText/ParserCore/HCParser.cs:195 in the FieldWorks repo; not checked here). It would need to: construct a degree-1 Morpher; gather a finite batch from its pending queue; call ParseWords; map each indexed result, error and trace back to the original request. Language reloads and option changes must wait for the batch to finish; selectors and custom rules must support concurrent calls. FieldWorks is not changed here.

🤖 Generated with Claude Code


This change is Reviewable

Require degree-1 compilation for batch parsing, preserve ordered per-word results and errors, and serialize traced batches. Keep alternative budgets on invocation-local rule wrappers and retain the existing constructor default.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.59091% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.88%. Comparing base (c245476) to head (d47ef0a).

Files with missing lines Patch % Lines
...chine.Morphology.HermitCrab/AnalysisStratumRule.cs 91.89% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   74.57%   74.88%   +0.31%     
==========================================
  Files         453      454       +1     
  Lines       38342    38397      +55     
  Branches     5284     5290       +6     
==========================================
+ Hits        28593    28755     +162     
+ Misses       8583     8488      -95     
+ Partials     1166     1154      -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

throw new ArgumentNullException(nameof(words));
if (maxWordConcurrency < 1)
throw new ArgumentOutOfRangeException(nameof(maxWordConcurrency));
if (MaxDegreeOfParallelism != 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: F1. ParseWords throws for any Morpher built with the default maxDegreeOfParallelism (0). So new Morpher(tm, lang).ParseWords(words) fails, even though the MaxDegreeOfParallelism doc in this file (L117-124) says that value "must remain a performance knob only". Running the batch serially, or with word concurrency 1, when degree != 1 would keep that contract. If the throw is deliberate, the doc should say so.

{
if (words == null)
throw new ArgumentNullException(nameof(words));
if (maxWordConcurrency < 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F2. A maxWordConcurrency below 1 throws ArgumentOutOfRangeException. Everywhere else in this class a parallelism below 1 means unbounded (MaxDegreeOfParallelism doc, CreateParallelOptions), and ParseWords_ValidatesBatchArguments asserts the throw for 0 and -1. A caller following that convention gets an exception.

if (maxAlternatives > 0)
{
// Limits belong to this invocation; compiled child rules remain shared and read-only.
morphologicalRules = CreateMorphologicalRules(_mrulesRule.Rules);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F3. With MaxAlternatives > 0, every Apply call copies both rule lists into a new cascade and a new RuleBatch. AnalysisLanguageRule makes that call once per input word per stratum, so the allocations grow with ambiguity. Building the capped pair once per stratum per parse, before the inData loop, gives the same limits. Unverified cost: not measured.

@claude

claude Bot commented Oct 29, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve with fixes. The per-invocation MaxAlternatives change looks correct to me, and it removes the shared-state write that made concurrent parses unsafe.
  2. Most important: F1 (src/SIL.Machine.Morphology.HermitCrab/Morpher.cs:239). ParseWords rejects the default-constructed Morpher, which contradicts the "performance knob only" contract on MaxDegreeOfParallelism.
  3. Counts: Critical 0, Important 1 (F1), Low 2 (F2, F3).
  4. Ran: the 25 new HermitCrab tests, filtered with dotnet test, passed 25/25 on net10.0 locally. gh pr checks 582: both Builds and Comment hygiene (advisory) passed; NuGet package is pending.
  5. Not verified: whether ParseWords_UsesBoundedWordConcurrency stays stable on a starved runner; I could not run it with a reduced processor count here. I did not fully audit whether analysis-side phonological rules are safe when applied concurrently to themselves, which this PR enables for the first time. A grep found no mutable instance fields in the HermitCrab, Matching, Rules, or FiniteState rule code.

Public API: adds Morpher.ParseWords and WordParseResult. AnalysisStratumRule.MaxAlternatives is removed, but that class is internal. Target framework, package, and sillsdev/machine.py parity: none verified.

Findings: F1 new, F2 new, F3 new, unverified (cost not measured).

Reviewed at d47ef0a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants