Skip to content

Add bounded batch parsing to HermitCrab - #582

Open
johnml1135 wants to merge 4 commits into
masterfrom
feat/hc-parse-words-batch
Open

johnml1135 wants to merge 4 commits 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. One breaking change: MaxAlternatives is now a constructor argument with no setter, fixed for the Morpher's lifetime, so the rules read it directly and parsing writes no shared compiled rule state (AnalysisStratumRule.cs, AnalysisLanguageRule.cs). Callers that set it must move it to the constructor and recompile; FieldWorks' HCParser.cs becomes new Morpher(m_traceManager, m_language, 0, maxAlternatives). ParseWords also takes an optional progress callback, called on the parsing threads when each word starts and finishes.

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

@codecov-commenter

codecov-commenter commented Oct 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.36842% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.08%. Comparing base (9f284a3) to head (45a3205).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
...chine.Morphology.HermitCrab/AnalysisStratumRule.cs 92.68% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   74.93%   75.08%   +0.14%     
==========================================
  Files         454      456       +2     
  Lines       38490    38569      +79     
  Branches     5322     5331       +9     
==========================================
+ Hits        28843    28958     +115     
+ Misses       8495     8463      -32     
+ Partials     1152     1148       -4     

☔ 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.

@claude

This comment has been minimized.

johnml1135 and others added 2 commits November 2, 2026 21:01
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.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135
johnml1135 force-pushed the feat/hc-parse-words-batch branch from d47ef0a to 9485394 Compare November 3, 2026 02:03
@claude

This comment has been minimized.

@johnml1135
johnml1135 requested a review from jtmaxwell3 November 3, 2026 02:11
@jtmaxwell3

Copy link
Copy Markdown
Collaborator

FieldWorks currently displays what word is being parsed and the number of words remaining in the lower left corner of the display.  This is very useful feedback for a long running process. Seeing what word is being parsed lets you see which words are taking a long time. Maybe ParseWords could take an optional callback function that is called when a word starts parsing and when it stops? The set of words currently being parsed could be displayed in the lower left corner.

@jtmaxwell3

Copy link
Copy Markdown
Collaborator

An alternative to passing limits like MaxAlternatives in as an argument to internal functions is to make them be readonly once Morpher has been initialized. In FieldWorks, the Hermit Crab grammar gets recompiled whenever any of the parameters are changed. Are there situations where we want to change MaxAlternatives on the fly? If so, maybe we should make it be an argument to ParseWord(s) rather than a variable in Morpher.

@johnml1135

Copy link
Copy Markdown
Collaborator Author

FieldWorks currently displays what word is being parsed and the number of words remaining…

Done in 4adfd57. ParseWords takes an optional callback that is called when each word starts and finishes, with its input index, surface form and (on finish) its WordParseResult. Callbacks run on the parsing threads and may overlap, so FieldWorks needs to marshal to the UI thread and keep them quick. If a callback fails, every word still parses and gets its finish notification, but ParseWords then throws instead of returning its result list.

@johnml1135

Copy link
Copy Markdown
Collaborator Author

An alternative to passing limits like MaxAlternatives in as an argument to internal functions is to make them be readonly once Morpher has been initialized…

Agreed, and done in 4adfd57. MaxAlternatives is now a constructor argument and stays fixed for the Morpher's lifetime. The rules read it directly instead of receiving it on every call. The public setter is intentionally removed, so callers that set it must move to the constructor and recompile. For FieldWorks (C# 8, so no init-only setters) that means new Morpher(m_traceManager, m_language, 0, maxAlternatives) in HCParser.cs in place of the initializer entry. Degree 0 keeps today's single-word behaviour; adopting ParseWords later (LT-22885) uses degree 1.

@claude

This comment has been minimized.

…ed progress callbacks.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve. The change since the last round fixes F4, and it adds no new findings.
  2. Most important: F4 (src/SIL.Machine.Morphology.HermitCrab/Morpher.cs:310) is addressed. A failing progress callback now throws WordParseProgressException, and its Results keeps the result of every word in input order.
  3. Counts: Critical 0, Important 0, Low 0 open (F4 addressed).
  4. Ran: dotnet test on the HermitCrab tests filtered to MorpherTests, Release, net10.0 locally: 48/48 passed. gh pr checks 582: Build on ubuntu-22.04 and Comment hygiene (advisory) passed. Build on windows-latest and claude-review are pending.
  5. Not verified: whether ParseWords_UsesBoundedWordConcurrency stays stable on a starved runner (still not run with a reduced processor count), and the FieldWorks HCParser.cs change, which lives outside this repo.

Public API: adds the public WordParseProgressException (derives from AggregateException, internal constructor, Results property), which is now what ParseWords throws for callback failures. The rest is unchanged since the last round: Morpher.ParseWords, WordParseResult, the Morpher(ITraceManager, Language, int, int) constructor, and the removed public MaxAlternatives setter, which breaks source and binary compatibility. Target framework and sillsdev/machine.py parity: none verified.

Findings: F1 addressed, F2 withdrawn, F3 accepted (deferred until measured; unverified cost), F4 addressed.

Reviewed at 45a3205

@jtmaxwell3

Copy link
Copy Markdown
Collaborator

I don't think that you need to throw WordParseProgressException at the end of ParseWords. Just return ordered. ordered has the exceptions stored in WordParseResults. Throwing WordParseProgressException just complicates the call to ParseWords.

This branch has not been deployed

No deployments
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.

4 participants