From 649df84eca8dd3a263897759ef49e98a4c7b4fbf Mon Sep 17 00:00:00 2001 From: Enkidu93 Date: Thu, 14 Sep 2023 14:03:28 -0400 Subject: [PATCH 1/7] Receive buildOptions in NMT builds and pass on to machine.py (To be tweaked after @mshannon-sil determines a strategy on his end) --ECL --- .../Services/ClearMLNmtEngineBuildJob.cs | 2 ++ .../Services/ClearMLNmtEngineService.cs | 3 ++- src/SIL.Machine.AspNetCore/Services/ClearMLService.cs | 4 +++- src/SIL.Machine.AspNetCore/Services/IClearMLService.cs | 1 + .../Services/ITranslationEngineService.cs | 1 + .../Services/ServalTranslationEngineServiceV1.cs | 8 +++++++- .../Services/SmtTransferEngineService.cs | 6 ++++-- .../Services/TranslationEngineServiceBase.cs | 10 ++++++++-- .../Services/ClearMLNmtEngineServiceTests.cs | 3 ++- .../Services/ClearMLServiceTests.cs | 3 ++- .../Services/SmtTransferEngineServiceTests.cs | 6 +++--- 11 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineBuildJob.cs b/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineBuildJob.cs index c171c0fde..7b11a1c50 100644 --- a/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineBuildJob.cs +++ b/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineBuildJob.cs @@ -34,6 +34,7 @@ ICorpusService corpusService public async Task RunAsync( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken ) @@ -68,6 +69,7 @@ CancellationToken cancellationToken engine.SourceLanguage, engine.TargetLanguage, _sharedFileService.GetBaseUri().ToString(), + buildOptions, cancellationToken ); await _clearMLService.EnqueueTaskAsync(clearMLTaskId, CancellationToken.None); diff --git a/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineService.cs b/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineService.cs index 42eb064e1..d97f21dd3 100644 --- a/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineService.cs +++ b/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineService.cs @@ -42,10 +42,11 @@ public override async Task DeleteAsync(string engineId, CancellationToken cancel protected override Expression> GetJobExpression( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora ) { // Token "None" is used here because hangfire injects the proper cancellation token - return r => r.RunAsync(engineId, buildId, corpora, CancellationToken.None); + return r => r.RunAsync(engineId, buildId, buildOptions, corpora, CancellationToken.None); } } diff --git a/src/SIL.Machine.AspNetCore/Services/ClearMLService.cs b/src/SIL.Machine.AspNetCore/Services/ClearMLService.cs index a798482c5..422123808 100644 --- a/src/SIL.Machine.AspNetCore/Services/ClearMLService.cs +++ b/src/SIL.Machine.AspNetCore/Services/ClearMLService.cs @@ -83,6 +83,7 @@ public async Task CreateTaskAsync( string sourceLanguageTag, string targetLanguageTag, string sharedFileUri, + string buildOptions, CancellationToken cancellationToken = default ) { @@ -96,7 +97,8 @@ public async Task CreateTaskAsync( + $" 'trg_lang': '{ConvertLanguageTag(targetLanguageTag)}',\n" + $" 'max_steps': {_options.CurrentValue.MaxSteps},\n" + $" 'shared_file_uri': '{sharedFileUri}',\n" - + $" 'clearml': True\n" + + $" 'clearml': True,\n" + + $" 'build_options': '{buildOptions}'\n" + "}\n" + "run(args)\n"; diff --git a/src/SIL.Machine.AspNetCore/Services/IClearMLService.cs b/src/SIL.Machine.AspNetCore/Services/IClearMLService.cs index 9ef4447d7..1dc68ca82 100644 --- a/src/SIL.Machine.AspNetCore/Services/IClearMLService.cs +++ b/src/SIL.Machine.AspNetCore/Services/IClearMLService.cs @@ -17,6 +17,7 @@ Task CreateTaskAsync( string sourceLanguageTag, string targetLanguageTag, string sharedFileUri, + string buildOptions, CancellationToken cancellationToken = default ); Task EnqueueTaskAsync(string id, CancellationToken cancellationToken = default); diff --git a/src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs b/src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs index 55bc419e2..66776f66e 100644 --- a/src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs +++ b/src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs @@ -33,6 +33,7 @@ Task TrainSegmentPairAsync( Task StartBuildAsync( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken = default ); diff --git a/src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs b/src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs index 28d95580e..bef1aee21 100644 --- a/src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs +++ b/src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs @@ -96,7 +96,13 @@ public override async Task StartBuild(StartBuildRequest request, ServerCa Models.Corpus[] corpora = request.Corpora.Select(Map).ToArray(); try { - await engineService.StartBuildAsync(request.EngineId, request.BuildId, corpora, context.CancellationToken); + await engineService.StartBuildAsync( + request.EngineId, + request.BuildId, + request.Options, + corpora, + context.CancellationToken + ); } catch (InvalidOperationException e) { diff --git a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs index 2cfccdc7e..06d1bf858 100644 --- a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs +++ b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs @@ -137,6 +137,7 @@ await _trainSegmentPairs.InsertAsync( public override async Task StartBuildAsync( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken = default ) @@ -145,7 +146,7 @@ public override async Task StartBuildAsync( IDistributedReaderWriterLock @lock = await LockFactory.CreateAsync(engineId, cancellationToken); await using (await @lock.WriterLockAsync(cancellationToken: cancellationToken)) { - await StartBuildInternalAsync(engineId, buildId, corpora, cancellationToken); + await StartBuildInternalAsync(engineId, buildId, buildOptions, corpora, cancellationToken); state.LastUsedTime = DateTime.UtcNow; } } @@ -164,10 +165,11 @@ public override async Task CancelBuildAsync(string engineId, CancellationToken c protected override Expression> GetJobExpression( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora ) { // Token "None" is used here because hangfire injects the proper cancellation token - return r => r.RunAsync(engineId, buildId, corpora, CancellationToken.None); + return r => r.RunAsync(engineId, buildId, corpora, CancellationToken.None); //Not passing build options to SMT yet; is there a need? } } diff --git a/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs b/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs index bb11a4395..cb89fdb00 100644 --- a/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs +++ b/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs @@ -56,6 +56,7 @@ public virtual async Task DeleteAsync(string engineId, CancellationToken cancell public virtual async Task StartBuildAsync( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken = default ) @@ -63,7 +64,7 @@ public virtual async Task StartBuildAsync( IDistributedReaderWriterLock @lock = await LockFactory.CreateAsync(engineId, cancellationToken); await using (await @lock.WriterLockAsync(cancellationToken: cancellationToken)) { - await StartBuildInternalAsync(engineId, buildId, corpora, cancellationToken); + await StartBuildInternalAsync(engineId, buildId, buildOptions, corpora, cancellationToken); } } @@ -109,12 +110,14 @@ public virtual Task TrainSegmentPairAsync( protected abstract Expression> GetJobExpression( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora ); protected async Task StartBuildInternalAsync( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken ) @@ -130,7 +133,10 @@ await Engines.ExistsAsync( throw new InvalidOperationException("Engine is already building or pending."); // Schedule the job to occur way in the future, just so we can get the job id. - string jobId = _jobClient.Schedule(GetJobExpression(engineId, buildId, corpora), TimeSpan.FromDays(10000)); + string jobId = _jobClient.Schedule( + GetJobExpression(engineId, buildId, buildOptions, corpora), + TimeSpan.FromDays(10000) + ); try { await Engines.UpdateAsync( diff --git a/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLNmtEngineServiceTests.cs b/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLNmtEngineServiceTests.cs index 614dabc52..656b7571e 100644 --- a/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLNmtEngineServiceTests.cs +++ b/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLNmtEngineServiceTests.cs @@ -15,6 +15,7 @@ public async Task CancelBuildAsync() "es", "en", "memory:///", + "{}", Arg.Any() ) .Returns(Task.FromResult("task1")); @@ -39,7 +40,7 @@ public async Task CancelBuildAsync() env.ClearMLService .GetTaskByIdAsync("task1", Arg.Any()) .Returns(Task.FromResult(task)); - await env.Service.StartBuildAsync("engine1", "build1", Array.Empty()); + await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty()); await env.WaitForBuildToStartAsync(); TranslationEngine engine = env.Engines.Get("engine1"); Assert.That(engine.BuildState, Is.EqualTo(BuildState.Active)); diff --git a/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLServiceTests.cs b/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLServiceTests.cs index 0fcdcb317..b81ef54f4 100644 --- a/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLServiceTests.cs +++ b/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLServiceTests.cs @@ -42,7 +42,8 @@ public async Task CreateTaskAsync() "engine1", "es", "en", - "s3://aqua-ml-data" + "s3://aqua-ml-data", + "{}" ); Assert.That(projectId, Is.EqualTo("projectId")); mockHttp.VerifyNoOutstandingExpectation(); diff --git a/tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs b/tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs index 7c69e79e4..d1e67004f 100644 --- a/tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs +++ b/tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs @@ -13,7 +13,7 @@ public async Task StartBuildAsync() Assert.That(engine.BuildRevision, Is.EqualTo(1)); //For testing purposes BuildRevision is set to 1 (i.e., an already built engine) // ensure that the SMT model was loaded before training await env.Service.TranslateAsync("engine1", n: 1, "esto es una prueba."); - await env.Service.StartBuildAsync("engine1", "build1", Array.Empty()); + await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty()); await env.WaitForBuildToFinishAsync(); await env.SmtBatchTrainer .Received() @@ -46,7 +46,7 @@ await env.SmtBatchTrainer.TrainAsync( ct.ThrowIfCancellationRequested(); }) ); - await env.Service.StartBuildAsync("engine1", "build1", Array.Empty()); + await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty()); await env.WaitForBuildToStartAsync(); TranslationEngine engine = env.Engines.Get("engine1"); Assert.That(engine.BuildState, Is.EqualTo(BuildState.Active)); @@ -74,7 +74,7 @@ public async Task StartBuildAsync_RestartUnfinishedBuild() Thread.Sleep(100); } }); - await env.Service.StartBuildAsync("engine1", "build1", Array.Empty()); + await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty()); await env.WaitForBuildToStartAsync(); TranslationEngine engine = env.Engines.Get("engine1"); Assert.That(engine.BuildState, Is.EqualTo(BuildState.Active)); From 2d25a42123937827dd8576e92cb633c2f55a26ea Mon Sep 17 00:00:00 2001 From: Enkidu93 Date: Thu, 14 Sep 2023 17:05:15 -0400 Subject: [PATCH 2/7] Pass buildOptions to SMT --ECL --- .../Services/SmtTransferEngineBuildJob.cs | 4 +++- .../Services/SmtTransferEngineService.cs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs index 5edc3ac57..7e145d687 100644 --- a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs +++ b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs @@ -38,18 +38,20 @@ ILogger logger public async Task RunAsync( string engineId, string buildId, + string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken ) { IDistributedReaderWriterLock rwLock = await _lockFactory.CreateAsync(engineId, cancellationToken); - var tokenizer = new LatinWordTokenizer(); var detokenizer = new LatinWordDetokenizer(); ITrainer? smtModelTrainer = null; ITrainer? truecaseTrainer = null; try { + JsonObject? buildOptionsObject = JsonSerializer.Deserialize(buildOptions); //Use/fields TBD + var stopwatch = new Stopwatch(); TranslationEngine? engine; await using (await rwLock.WriterLockAsync(cancellationToken: cancellationToken)) diff --git a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs index 06d1bf858..ccee66307 100644 --- a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs +++ b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs @@ -170,6 +170,6 @@ IReadOnlyList corpora ) { // Token "None" is used here because hangfire injects the proper cancellation token - return r => r.RunAsync(engineId, buildId, corpora, CancellationToken.None); //Not passing build options to SMT yet; is there a need? + return r => r.RunAsync(engineId, buildId, buildOptions, corpora, CancellationToken.None); } } From 01faccfb4754d5c9a29fd26d1b8b9860fc961f8c Mon Sep 17 00:00:00 2001 From: Enkidu93 Date: Mon, 25 Sep 2023 15:04:00 -0400 Subject: [PATCH 3/7] Add strict parsing option to SMT build jobs via buildOptions parameter Fixes https://github.com/sillsdev/serval/issues/2 --- .../Services/SmtTransferEngineBuildJob.cs | 19 +++++++++- .../Services/TranslationEngineServiceBase.cs | 14 ++++--- src/SIL.Machine/Corpora/CorporaExtensions.cs | 5 ++- src/SIL.Machine/Corpora/ParallelTextCorpus.cs | 38 ++++++++++++++++--- 4 files changed, 61 insertions(+), 15 deletions(-) diff --git a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs index 7e145d687..87ad8cca8 100644 --- a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs +++ b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs @@ -50,8 +50,22 @@ CancellationToken cancellationToken ITrainer? truecaseTrainer = null; try { + if (buildOptions == "") + { + buildOptions = "{}"; + } + _logger.LogInformation($"Build options: {buildOptions}"); JsonObject? buildOptionsObject = JsonSerializer.Deserialize(buildOptions); //Use/fields TBD - + bool useStrictParsing = false; + try + { + useStrictParsing = bool.Parse((string?)buildOptionsObject?["useStrictParsing"] ?? "false"); + } + catch (FormatException) + { + _logger.LogInformation($"Parsing of {buildOptions} failed"); + } + _logger.LogInformation($"Strict parsing: {useStrictParsing}"); var stopwatch = new Stopwatch(); TranslationEngine? engine; await using (await rwLock.WriterLockAsync(cancellationToken: cancellationToken)) @@ -77,10 +91,11 @@ CancellationToken cancellationToken foreach (Corpus corpus in corpora) { ITextCorpus sc = _corpusService.CreateTextCorpus(corpus.SourceFiles); + ITextCorpus tc = _corpusService.CreateTextCorpus(corpus.TargetFiles); targetCorpora.Add(tc); - parallelCorpora.Add(sc.AlignRows(tc)); + parallelCorpora.Add(sc.AlignRows(tc, useStrictParsing: useStrictParsing)); } IParallelTextCorpus parallelCorpus = parallelCorpora.Flatten(); diff --git a/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs b/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs index cb89fdb00..1ade7014e 100644 --- a/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs +++ b/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs @@ -1,4 +1,6 @@ -namespace SIL.Machine.AspNetCore.Services; +using Serval.Translation.V1; + +namespace SIL.Machine.AspNetCore.Services; public abstract class TranslationEngineServiceBase : ITranslationEngineService { @@ -57,7 +59,7 @@ public virtual async Task StartBuildAsync( string engineId, string buildId, string buildOptions, - IReadOnlyList corpora, + IReadOnlyList corpora, CancellationToken cancellationToken = default ) { @@ -77,7 +79,7 @@ public virtual async Task CancelBuildAsync(string engineId, CancellationToken ca } } - public virtual Task> TranslateAsync( + public virtual Task> TranslateAsync( string engineId, int n, string segment, @@ -87,7 +89,7 @@ public virtual Task> TranslateAsync( throw new NotSupportedException(); } - public virtual Task GetWordGraphAsync( + public virtual Task GetWordGraphAsync( string engineId, string segment, CancellationToken cancellationToken = default @@ -111,14 +113,14 @@ protected abstract Expression> GetJobExpression( string engineId, string buildId, string buildOptions, - IReadOnlyList corpora + IReadOnlyList corpora ); protected async Task StartBuildInternalAsync( string engineId, string buildId, string buildOptions, - IReadOnlyList corpora, + IReadOnlyList corpora, CancellationToken cancellationToken ) { diff --git a/src/SIL.Machine/Corpora/CorporaExtensions.cs b/src/SIL.Machine/Corpora/CorporaExtensions.cs index 3e2800d12..55704ade7 100644 --- a/src/SIL.Machine/Corpora/CorporaExtensions.cs +++ b/src/SIL.Machine/Corpora/CorporaExtensions.cs @@ -235,10 +235,11 @@ public static IParallelTextCorpus AlignRows( IAlignmentCorpus alignmentCorpus = null, bool allSourceRows = false, bool allTargetRows = false, - IComparer rowRefComparer = null + IComparer rowRefComparer = null, + bool useStrictParsing = false ) { - return new ParallelTextCorpus(sourceCorpus, targetCorpus, alignmentCorpus, rowRefComparer) + return new ParallelTextCorpus(sourceCorpus, targetCorpus, alignmentCorpus, rowRefComparer, useStrictParsing) { AllSourceRows = allSourceRows, AllTargetRows = allTargetRows diff --git a/src/SIL.Machine/Corpora/ParallelTextCorpus.cs b/src/SIL.Machine/Corpora/ParallelTextCorpus.cs index ac743933a..d1017683a 100644 --- a/src/SIL.Machine/Corpora/ParallelTextCorpus.cs +++ b/src/SIL.Machine/Corpora/ParallelTextCorpus.cs @@ -13,13 +13,15 @@ public ParallelTextCorpus( ITextCorpus sourceCorpus, ITextCorpus targetCorpus, IAlignmentCorpus alignmentCorpus = null, - IComparer rowRefComparer = null + IComparer rowRefComparer = null, + bool useStrictParsing = false ) { SourceCorpus = sourceCorpus; TargetCorpus = targetCorpus; AlignmentCorpus = alignmentCorpus ?? new DictionaryAlignmentCorpus(); RowRefComparer = rowRefComparer ?? new DefaultRowRefComparer(); + UseStrictParsing = useStrictParsing; } public bool IsSourceTokenized => SourceCorpus.IsTokenized; @@ -28,6 +30,8 @@ public ParallelTextCorpus( public bool AllSourceRows { get; set; } public bool AllTargetRows { get; set; } + public bool UseStrictParsing { get; set; } + public ITextCorpus SourceCorpus { get; } public ITextCorpus TargetCorpus { get; } public IAlignmentCorpus AlignmentCorpus { get; } @@ -94,7 +98,20 @@ public IEnumerable GetRows() bool trgCompleted = !trgEnumerator.MoveNext(); while (!srcCompleted && !trgCompleted) { - int compare1 = RowRefComparer.Compare(srcEnumerator.Current.Ref, trgEnumerator.Current.Ref); + int compare1 = 0; + if (UseStrictParsing) + { + try + { + compare1 = RowRefComparer.Compare(srcEnumerator.Current.Ref, trgEnumerator.Current.Ref); + } + catch (ArgumentException) + { + throw new FormatException( + $"Invalid format in {srcEnumerator.Current.TextId} and {trgEnumerator.Current.TextId}. Mismatched key formats \"{srcEnumerator.Current.Ref}\" and \"{trgEnumerator.Current.Ref}\". There may be an extraneous tab, missing ref, or inconsistent use of user-defined refs. If you'd like to ignore format errors, disable strict parsing." + ); + } + } if (compare1 < 0) { if (!AllTargetRows && srcEnumerator.Current.IsInRange) @@ -378,9 +395,20 @@ private IEnumerable CreateRows( private bool CheckSameRefRows(List sameRefRows, TextRow otherRow) { - if (sameRefRows.Count > 0 && RowRefComparer.Compare(sameRefRows[0].Ref, otherRow.Ref) != 0) - sameRefRows.Clear(); - + try + { + if (sameRefRows.Count > 0 && RowRefComparer.Compare(sameRefRows[0].Ref, otherRow.Ref) != 0) + sameRefRows.Clear(); + } + catch (ArgumentException) + { + if (UseStrictParsing) + { + throw new FormatException( + $"Invalid format in {sameRefRows[0].TextId} and {otherRow.TextId}. Mismatched key formats \"{sameRefRows[0].Ref}\" and \"{otherRow.Ref}\". There may be an extraneous tab, missing ref, or inconsistent use of user-defined refs. If you'd like to ignore format errors, disable strict parsing." + ); + } + } return sameRefRows.Count > 0; } From bc3306bd7609c4f1caed957a13bbfa9eb9372395 Mon Sep 17 00:00:00 2001 From: Enkidu93 Date: Tue, 26 Sep 2023 15:13:42 -0400 Subject: [PATCH 4/7] Removed flag logic and added custom exception --- .../Corpora/CorpusAlignmentException.cs | 12 +++++++ src/SIL.Machine/Corpora/ParallelTextCorpus.cs | 33 ++++++++++++++----- 2 files changed, 37 insertions(+), 8 deletions(-) create mode 100644 src/SIL.Machine/Corpora/CorpusAlignmentException.cs diff --git a/src/SIL.Machine/Corpora/CorpusAlignmentException.cs b/src/SIL.Machine/Corpora/CorpusAlignmentException.cs new file mode 100644 index 000000000..c86dd8cfd --- /dev/null +++ b/src/SIL.Machine/Corpora/CorpusAlignmentException.cs @@ -0,0 +1,12 @@ +using System; + +namespace SIL.Machine.Corpora +{ + public class CorpusAlignmentException : Exception + { + public CorpusAlignmentException(string sourceRef, string targetRef) + : base( + $"Invalid format in {sourceRef} and {targetRef}. Mismatched key formats \"{sourceRef}\" and \"{targetRef}\". There may be an extraneous tab, missing ref, or inconsistent use of user-defined refs." + ) { } + } +} diff --git a/src/SIL.Machine/Corpora/ParallelTextCorpus.cs b/src/SIL.Machine/Corpora/ParallelTextCorpus.cs index ac743933a..330d5a999 100644 --- a/src/SIL.Machine/Corpora/ParallelTextCorpus.cs +++ b/src/SIL.Machine/Corpora/ParallelTextCorpus.cs @@ -13,7 +13,8 @@ public ParallelTextCorpus( ITextCorpus sourceCorpus, ITextCorpus targetCorpus, IAlignmentCorpus alignmentCorpus = null, - IComparer rowRefComparer = null + IComparer rowRefComparer = null, + bool useStrictParsing = false ) { SourceCorpus = sourceCorpus; @@ -94,7 +95,7 @@ public IEnumerable GetRows() bool trgCompleted = !trgEnumerator.MoveNext(); while (!srcCompleted && !trgCompleted) { - int compare1 = RowRefComparer.Compare(srcEnumerator.Current.Ref, trgEnumerator.Current.Ref); + int compare1 = 0; if (compare1 < 0) { if (!AllTargetRows && srcEnumerator.Current.IsInRange) @@ -176,9 +177,19 @@ ParallelTextRow row in CreateTargetRows( int compare2; do { - compare2 = alignmentEnumerator.MoveNext() - ? RowRefComparer.Compare(srcEnumerator.Current.Ref, alignmentEnumerator.Current.Ref) - : 1; + try + { + compare2 = alignmentEnumerator.MoveNext() + ? RowRefComparer.Compare(srcEnumerator.Current.Ref, alignmentEnumerator.Current.Ref) + : 1; + } + catch (ArgumentException) + { + throw new CorpusAlignmentException( + srcEnumerator.Current.Ref.ToString(), + trgEnumerator.Current.Ref.ToString() + ); + } } while (compare2 < 0); if ( @@ -378,9 +389,15 @@ private IEnumerable CreateRows( private bool CheckSameRefRows(List sameRefRows, TextRow otherRow) { - if (sameRefRows.Count > 0 && RowRefComparer.Compare(sameRefRows[0].Ref, otherRow.Ref) != 0) - sameRefRows.Clear(); - + try + { + if (sameRefRows.Count > 0 && RowRefComparer.Compare(sameRefRows[0].Ref, otherRow.Ref) != 0) + sameRefRows.Clear(); + } + catch (ArgumentException) + { + throw new CorpusAlignmentException(sameRefRows[0].Ref.ToString(), otherRow.Ref.ToString()); + } return sameRefRows.Count > 0; } From 0cb6f2ffd9182a4fe47b5a163b78cf2930013dd1 Mon Sep 17 00:00:00 2001 From: Enkidu93 Date: Tue, 26 Sep 2023 15:52:57 -0400 Subject: [PATCH 5/7] Remove buildOptions-related changes --- .../Services/ClearMLNmtEngineBuildJob.cs | 2 -- .../Services/ClearMLNmtEngineService.cs | 3 +-- src/SIL.Machine.AspNetCore/Services/ClearMLService.cs | 2 -- src/SIL.Machine.AspNetCore/Services/IClearMLService.cs | 1 - .../Services/ITranslationEngineService.cs | 1 - .../Services/SmtTransferEngineBuildJob.cs | 3 --- .../Services/SmtTransferEngineService.cs | 6 ++---- .../Services/TranslationEngineServiceBase.cs | 10 ++-------- 8 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineBuildJob.cs b/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineBuildJob.cs index 7b11a1c50..c171c0fde 100644 --- a/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineBuildJob.cs +++ b/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineBuildJob.cs @@ -34,7 +34,6 @@ ICorpusService corpusService public async Task RunAsync( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken ) @@ -69,7 +68,6 @@ CancellationToken cancellationToken engine.SourceLanguage, engine.TargetLanguage, _sharedFileService.GetBaseUri().ToString(), - buildOptions, cancellationToken ); await _clearMLService.EnqueueTaskAsync(clearMLTaskId, CancellationToken.None); diff --git a/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineService.cs b/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineService.cs index d97f21dd3..42eb064e1 100644 --- a/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineService.cs +++ b/src/SIL.Machine.AspNetCore/Services/ClearMLNmtEngineService.cs @@ -42,11 +42,10 @@ public override async Task DeleteAsync(string engineId, CancellationToken cancel protected override Expression> GetJobExpression( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora ) { // Token "None" is used here because hangfire injects the proper cancellation token - return r => r.RunAsync(engineId, buildId, buildOptions, corpora, CancellationToken.None); + return r => r.RunAsync(engineId, buildId, corpora, CancellationToken.None); } } diff --git a/src/SIL.Machine.AspNetCore/Services/ClearMLService.cs b/src/SIL.Machine.AspNetCore/Services/ClearMLService.cs index 422123808..2cb7b7f89 100644 --- a/src/SIL.Machine.AspNetCore/Services/ClearMLService.cs +++ b/src/SIL.Machine.AspNetCore/Services/ClearMLService.cs @@ -83,7 +83,6 @@ public async Task CreateTaskAsync( string sourceLanguageTag, string targetLanguageTag, string sharedFileUri, - string buildOptions, CancellationToken cancellationToken = default ) { @@ -98,7 +97,6 @@ public async Task CreateTaskAsync( + $" 'max_steps': {_options.CurrentValue.MaxSteps},\n" + $" 'shared_file_uri': '{sharedFileUri}',\n" + $" 'clearml': True,\n" - + $" 'build_options': '{buildOptions}'\n" + "}\n" + "run(args)\n"; diff --git a/src/SIL.Machine.AspNetCore/Services/IClearMLService.cs b/src/SIL.Machine.AspNetCore/Services/IClearMLService.cs index 1dc68ca82..9ef4447d7 100644 --- a/src/SIL.Machine.AspNetCore/Services/IClearMLService.cs +++ b/src/SIL.Machine.AspNetCore/Services/IClearMLService.cs @@ -17,7 +17,6 @@ Task CreateTaskAsync( string sourceLanguageTag, string targetLanguageTag, string sharedFileUri, - string buildOptions, CancellationToken cancellationToken = default ); Task EnqueueTaskAsync(string id, CancellationToken cancellationToken = default); diff --git a/src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs b/src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs index 66776f66e..55bc419e2 100644 --- a/src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs +++ b/src/SIL.Machine.AspNetCore/Services/ITranslationEngineService.cs @@ -33,7 +33,6 @@ Task TrainSegmentPairAsync( Task StartBuildAsync( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken = default ); diff --git a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs index 7e145d687..6ad7ab411 100644 --- a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs +++ b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineBuildJob.cs @@ -38,7 +38,6 @@ ILogger logger public async Task RunAsync( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken ) @@ -50,8 +49,6 @@ CancellationToken cancellationToken ITrainer? truecaseTrainer = null; try { - JsonObject? buildOptionsObject = JsonSerializer.Deserialize(buildOptions); //Use/fields TBD - var stopwatch = new Stopwatch(); TranslationEngine? engine; await using (await rwLock.WriterLockAsync(cancellationToken: cancellationToken)) diff --git a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs index ccee66307..2cfccdc7e 100644 --- a/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs +++ b/src/SIL.Machine.AspNetCore/Services/SmtTransferEngineService.cs @@ -137,7 +137,6 @@ await _trainSegmentPairs.InsertAsync( public override async Task StartBuildAsync( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken = default ) @@ -146,7 +145,7 @@ public override async Task StartBuildAsync( IDistributedReaderWriterLock @lock = await LockFactory.CreateAsync(engineId, cancellationToken); await using (await @lock.WriterLockAsync(cancellationToken: cancellationToken)) { - await StartBuildInternalAsync(engineId, buildId, buildOptions, corpora, cancellationToken); + await StartBuildInternalAsync(engineId, buildId, corpora, cancellationToken); state.LastUsedTime = DateTime.UtcNow; } } @@ -165,11 +164,10 @@ public override async Task CancelBuildAsync(string engineId, CancellationToken c protected override Expression> GetJobExpression( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora ) { // Token "None" is used here because hangfire injects the proper cancellation token - return r => r.RunAsync(engineId, buildId, buildOptions, corpora, CancellationToken.None); + return r => r.RunAsync(engineId, buildId, corpora, CancellationToken.None); } } diff --git a/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs b/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs index cb89fdb00..bb11a4395 100644 --- a/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs +++ b/src/SIL.Machine.AspNetCore/Services/TranslationEngineServiceBase.cs @@ -56,7 +56,6 @@ public virtual async Task DeleteAsync(string engineId, CancellationToken cancell public virtual async Task StartBuildAsync( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken = default ) @@ -64,7 +63,7 @@ public virtual async Task StartBuildAsync( IDistributedReaderWriterLock @lock = await LockFactory.CreateAsync(engineId, cancellationToken); await using (await @lock.WriterLockAsync(cancellationToken: cancellationToken)) { - await StartBuildInternalAsync(engineId, buildId, buildOptions, corpora, cancellationToken); + await StartBuildInternalAsync(engineId, buildId, corpora, cancellationToken); } } @@ -110,14 +109,12 @@ public virtual Task TrainSegmentPairAsync( protected abstract Expression> GetJobExpression( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora ); protected async Task StartBuildInternalAsync( string engineId, string buildId, - string buildOptions, IReadOnlyList corpora, CancellationToken cancellationToken ) @@ -133,10 +130,7 @@ await Engines.ExistsAsync( throw new InvalidOperationException("Engine is already building or pending."); // Schedule the job to occur way in the future, just so we can get the job id. - string jobId = _jobClient.Schedule( - GetJobExpression(engineId, buildId, buildOptions, corpora), - TimeSpan.FromDays(10000) - ); + string jobId = _jobClient.Schedule(GetJobExpression(engineId, buildId, corpora), TimeSpan.FromDays(10000)); try { await Engines.UpdateAsync( From 95c4744723190fcfffba8fdffaa0051b44235b40 Mon Sep 17 00:00:00 2001 From: Enkidu93 Date: Tue, 26 Sep 2023 15:58:01 -0400 Subject: [PATCH 6/7] Removed mention of buildOptions from tests --- .../Services/ServalTranslationEngineServiceV1.cs | 8 +------- .../Services/ClearMLNmtEngineServiceTests.cs | 3 +-- .../Services/ClearMLServiceTests.cs | 3 +-- .../Services/SmtTransferEngineServiceTests.cs | 6 +++--- 4 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs b/src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs index bef1aee21..28d95580e 100644 --- a/src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs +++ b/src/SIL.Machine.AspNetCore/Services/ServalTranslationEngineServiceV1.cs @@ -96,13 +96,7 @@ public override async Task StartBuild(StartBuildRequest request, ServerCa Models.Corpus[] corpora = request.Corpora.Select(Map).ToArray(); try { - await engineService.StartBuildAsync( - request.EngineId, - request.BuildId, - request.Options, - corpora, - context.CancellationToken - ); + await engineService.StartBuildAsync(request.EngineId, request.BuildId, corpora, context.CancellationToken); } catch (InvalidOperationException e) { diff --git a/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLNmtEngineServiceTests.cs b/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLNmtEngineServiceTests.cs index 656b7571e..614dabc52 100644 --- a/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLNmtEngineServiceTests.cs +++ b/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLNmtEngineServiceTests.cs @@ -15,7 +15,6 @@ public async Task CancelBuildAsync() "es", "en", "memory:///", - "{}", Arg.Any() ) .Returns(Task.FromResult("task1")); @@ -40,7 +39,7 @@ public async Task CancelBuildAsync() env.ClearMLService .GetTaskByIdAsync("task1", Arg.Any()) .Returns(Task.FromResult(task)); - await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty()); + await env.Service.StartBuildAsync("engine1", "build1", Array.Empty()); await env.WaitForBuildToStartAsync(); TranslationEngine engine = env.Engines.Get("engine1"); Assert.That(engine.BuildState, Is.EqualTo(BuildState.Active)); diff --git a/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLServiceTests.cs b/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLServiceTests.cs index b81ef54f4..0fcdcb317 100644 --- a/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLServiceTests.cs +++ b/tests/SIL.Machine.AspNetCore.Tests/Services/ClearMLServiceTests.cs @@ -42,8 +42,7 @@ public async Task CreateTaskAsync() "engine1", "es", "en", - "s3://aqua-ml-data", - "{}" + "s3://aqua-ml-data" ); Assert.That(projectId, Is.EqualTo("projectId")); mockHttp.VerifyNoOutstandingExpectation(); diff --git a/tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs b/tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs index d1e67004f..7c69e79e4 100644 --- a/tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs +++ b/tests/SIL.Machine.AspNetCore.Tests/Services/SmtTransferEngineServiceTests.cs @@ -13,7 +13,7 @@ public async Task StartBuildAsync() Assert.That(engine.BuildRevision, Is.EqualTo(1)); //For testing purposes BuildRevision is set to 1 (i.e., an already built engine) // ensure that the SMT model was loaded before training await env.Service.TranslateAsync("engine1", n: 1, "esto es una prueba."); - await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty()); + await env.Service.StartBuildAsync("engine1", "build1", Array.Empty()); await env.WaitForBuildToFinishAsync(); await env.SmtBatchTrainer .Received() @@ -46,7 +46,7 @@ await env.SmtBatchTrainer.TrainAsync( ct.ThrowIfCancellationRequested(); }) ); - await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty()); + await env.Service.StartBuildAsync("engine1", "build1", Array.Empty()); await env.WaitForBuildToStartAsync(); TranslationEngine engine = env.Engines.Get("engine1"); Assert.That(engine.BuildState, Is.EqualTo(BuildState.Active)); @@ -74,7 +74,7 @@ public async Task StartBuildAsync_RestartUnfinishedBuild() Thread.Sleep(100); } }); - await env.Service.StartBuildAsync("engine1", "build1", "{}", Array.Empty()); + await env.Service.StartBuildAsync("engine1", "build1", Array.Empty()); await env.WaitForBuildToStartAsync(); TranslationEngine engine = env.Engines.Get("engine1"); Assert.That(engine.BuildState, Is.EqualTo(BuildState.Active)); From b41438da73c486c37d3b796c7fdabfa90db7a7dc Mon Sep 17 00:00:00 2001 From: Enkidu93 Date: Tue, 26 Sep 2023 16:06:07 -0400 Subject: [PATCH 7/7] Fix typo --- src/SIL.Machine/Corpora/ParallelTextCorpus.cs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/SIL.Machine/Corpora/ParallelTextCorpus.cs b/src/SIL.Machine/Corpora/ParallelTextCorpus.cs index 330d5a999..31baf1b79 100644 --- a/src/SIL.Machine/Corpora/ParallelTextCorpus.cs +++ b/src/SIL.Machine/Corpora/ParallelTextCorpus.cs @@ -13,8 +13,7 @@ public ParallelTextCorpus( ITextCorpus sourceCorpus, ITextCorpus targetCorpus, IAlignmentCorpus alignmentCorpus = null, - IComparer rowRefComparer = null, - bool useStrictParsing = false + IComparer rowRefComparer = null ) { SourceCorpus = sourceCorpus; @@ -96,6 +95,17 @@ public IEnumerable GetRows() while (!srcCompleted && !trgCompleted) { int compare1 = 0; + try + { + compare1 = RowRefComparer.Compare(srcEnumerator.Current.Ref, trgEnumerator.Current.Ref); + } + catch (ArgumentException) + { + throw new CorpusAlignmentException( + srcEnumerator.Current.Ref.ToString(), + trgEnumerator.Current.Ref.ToString() + ); + } if (compare1 < 0) { if (!AllTargetRows && srcEnumerator.Current.IsInRange)