Skip to content

Commit

Permalink
Code review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@pmachapman
pmachapman committed Nov 3, 2024
1 parent 28d587b commit 913650d
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 29 deletions.
56 changes: 35 additions & 21 deletions src/SIL.XForge.Scripture/Services/MachineProjectService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,7 @@ CancellationToken cancellationToken
}

// Ensure we have a translation engine id
string translationEngineId = preTranslate
? projectSecret.ServalData?.PreTranslationEngineId
: projectSecret.ServalData?.TranslationEngineId;
string translationEngineId = GetTranslationEngineId(projectSecret, preTranslate);
if (string.IsNullOrWhiteSpace(translationEngineId))
{
logger.LogInformation($"No Translation Engine Id specified for project {sfProjectId}");
Expand Down Expand Up @@ -627,12 +625,20 @@ await projectSecrets.UpdateAsync(
/// Creates or Updates a Parallel Corpus on Serval.
/// </summary>
/// <param name="translationEngineId">The translation engine identifier.</param>
/// <param name="parallelCorpusId">The parallel corpus identifier.</param>
/// <param name="name">The name of the parallel corpus.</param>
/// <param name="parallelCorpusId">
/// The parallel corpus to be updated. If <c>null</c> or empty, a new parallel corpus will be created.
/// </param>
/// <param name="name">
/// The name of the parallel corpus. This will only be used if the parallel corpus is being created.
/// </param>
/// <param name="sourceCorpusIds">The source corpus identifiers.</param>
/// <param name="targetCorpusIds">The target corpus identifiers.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>The parallel corpus identifier.</returns>
/// <returns>
/// The new or updated parallel corpus identifier. If <paramref name="parallelCorpusId"/> is not <c>null</c>,
/// this will be the same value as <paramref name="parallelCorpusId"/>. If <paramref name="parallelCorpusId"/>
/// is <c>null</c>, this will be the identifier of the new parallel corpus.
/// </returns>
/// <remarks>This can be mocked in unit tests.</remarks>
protected internal virtual async Task<string> CreateOrUpdateParallelCorpusAsync(
string translationEngineId,
Expand All @@ -645,6 +651,7 @@ CancellationToken cancellationToken
{
if (string.IsNullOrWhiteSpace(parallelCorpusId))
{
// Create a new parallel corpus
TranslationParallelCorpus parallelCorpus = await translationEnginesClient.AddParallelCorpusAsync(
translationEngineId,
new TranslationParallelCorpusConfig
Expand All @@ -659,6 +666,7 @@ CancellationToken cancellationToken
}
else
{
// Update the specified parallel corpus
await translationEnginesClient.UpdateParallelCorpusAsync(
translationEngineId,
parallelCorpusId,
Expand Down Expand Up @@ -692,9 +700,7 @@ CancellationToken cancellationToken
{
// Get the existing project secret, so we can see how to create the engine and update the Serval data
SFProjectSecret projectSecret = await projectSecrets.GetAsync(sfProject.Id);
string translationEngineId = preTranslate
? projectSecret.ServalData?.PreTranslationEngineId
: projectSecret.ServalData?.TranslationEngineId;
string translationEngineId = GetTranslationEngineId(projectSecret, preTranslate);
if (string.IsNullOrWhiteSpace(translationEngineId))
{
TranslationEngineConfig engineConfig = new TranslationEngineConfig
Expand Down Expand Up @@ -856,9 +862,7 @@ protected internal virtual async Task<string> EnsureTranslationEngineExistsAsync
CancellationToken cancellationToken
)
{
string translationEngineId = preTranslate
? projectSecret.ServalData?.PreTranslationEngineId
: projectSecret.ServalData?.TranslationEngineId;
string translationEngineId = GetTranslationEngineId(projectSecret, preTranslate);
if (!await TranslationEngineExistsAsync(projectDoc.Id, translationEngineId, preTranslate, cancellationToken))
{
// We do not have one, likely because the translation is a back translation
Expand Down Expand Up @@ -952,7 +956,7 @@ await projectSecrets.UpdateAsync(
// Ensure a translation engine id is present
if (string.IsNullOrWhiteSpace(translationEngineId))
{
throw new DataNotFoundException("The translation engine is not specified.");
throw new DataNotFoundException("Failed to create a translation engine.");
}

return translationEngineId;
Expand Down Expand Up @@ -1288,9 +1292,7 @@ CancellationToken cancellationToken
}

// Ensure we have a translation engine id
string translationEngineId = preTranslate
? projectSecret.ServalData?.PreTranslationEngineId
: projectSecret.ServalData?.TranslationEngineId;
string translationEngineId = GetTranslationEngineId(projectSecret, preTranslate);
if (string.IsNullOrWhiteSpace(translationEngineId))
{
logger.LogInformation($"No Translation Engine Id specified for project {sfProjectId}");
Expand Down Expand Up @@ -1345,7 +1347,7 @@ await translationEnginesClient.DeleteCorpusAsync(
}

/// <summary>
/// Synchronises the additional training data for a pre-translation project.
/// Synchronizes the additional training data for a pre-translation project.
/// </summary>
/// <param name="curUserId">The current user identifier</param>
/// <param name="project">The project.</param>
Expand All @@ -1354,7 +1356,11 @@ await translationEnginesClient.DeleteCorpusAsync(
/// <param name="additionalTrainingData">The additional training data.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>The updated <paramref name="additionalTrainingData"/>.</returns>
/// <remarks>This can be mocked in unit tests.</remarks>
/// <remarks>
/// If there are no TrainingDataFiles specified in <paramref name="buildConfig"/>, then the additional training
/// data corpora will be removed from Serval. Otherwise, the corpora will be created or updated as required.
/// This can be mocked in unit tests.
/// </remarks>
protected internal virtual async Task<ServalAdditionalTrainingData?> SyncAdditionalTrainingData(
string curUserId,
SFProject project,
Expand Down Expand Up @@ -1501,9 +1507,7 @@ CancellationToken cancellationToken
List<ServalCorpusSyncInfo> corporaSyncInfo = [];

// Ensure we have a translation engine ID
string translationEngineId = preTranslate
? projectSecret.ServalData?.PreTranslationEngineId
: projectSecret.ServalData?.TranslationEngineId;
string translationEngineId = GetTranslationEngineId(projectSecret, preTranslate);
if (string.IsNullOrWhiteSpace(translationEngineId))
{
throw new DataNotFoundException("The translation engine ID cannot be found.");
Expand Down Expand Up @@ -1987,6 +1991,16 @@ CancellationToken cancellationToken
return true;
}

/// <summary>
/// Gets the translation engine identifier from the project secret,
/// depending on whether we are pre-translating or not.
/// </summary>
/// <param name="projectSecret">The project secret.</param>
/// <param name="preTranslate">If <c>true</c>, we are pre-translating.</param>
/// <returns>The translation engine identifier.</returns>
private static string? GetTranslationEngineId(SFProjectSecret projectSecret, bool preTranslate) =>
preTranslate ? projectSecret.ServalData?.PreTranslationEngineId : projectSecret.ServalData?.TranslationEngineId;

/// <summary>
/// Records the Corpus Synchronization information.
/// </summary>
Expand Down
16 changes: 8 additions & 8 deletions test/SIL.XForge.Scripture.Tests/Services/MachineProjectServiceTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,9 @@ await env.Service.BuildProjectForBackgroundJobAsync(

env.MockLogger.AssertNoEvent(logEvent => logEvent.Exception == ex);
env.ExceptionHandler.DidNotReceiveWithAnyArgs().ReportException(ex);
Assert.IsNull(env.ProjectSecrets.Get(Project02).ServalData!.PreTranslationErrorMessage);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.PreTranslationErrorMessage);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.PreTranslationJobId);
Assert.IsNull(env.ProjectSecrets.Get(Project02).ServalData!.PreTranslationQueuedAt);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.PreTranslationQueuedAt);
}

[Test]
Expand Down Expand Up @@ -156,9 +156,9 @@ await env.Service.BuildProjectForBackgroundJobAsync(

env.MockLogger.AssertNoEvent(logEvent => logEvent.Exception == ex);
env.ExceptionHandler.DidNotReceiveWithAnyArgs().ReportException(ex);
Assert.IsNull(env.ProjectSecrets.Get(Project02).ServalData!.TranslationErrorMessage);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.TranslationErrorMessage);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.TranslationJobId);
Assert.IsNull(env.ProjectSecrets.Get(Project02).ServalData!.TranslationQueuedAt);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.TranslationQueuedAt);
}

[Test]
Expand All @@ -184,8 +184,8 @@ await env.Service.BuildProjectForBackgroundJobAsync(
);

env.ExceptionHandler.DidNotReceive().ReportException(Arg.Any<Exception>());
Assert.IsNull(env.ProjectSecrets.Get(Project02).ServalData!.PreTranslationQueuedAt);
Assert.IsNull(env.ProjectSecrets.Get(Project02).ServalData!.PreTranslationErrorMessage);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.PreTranslationQueuedAt);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.PreTranslationErrorMessage);
}

[Test]
Expand All @@ -211,8 +211,8 @@ await env.Service.BuildProjectForBackgroundJobAsync(
);

env.ExceptionHandler.DidNotReceive().ReportException(Arg.Any<Exception>());
Assert.IsNull(env.ProjectSecrets.Get(Project02).ServalData!.TranslationQueuedAt);
Assert.IsNull(env.ProjectSecrets.Get(Project02).ServalData!.TranslationErrorMessage);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.TranslationQueuedAt);
Assert.IsNull(env.ProjectSecrets.Get(Project01).ServalData!.TranslationErrorMessage);
}

[Test]
Expand Down

0 comments on commit 913650d

Please sign in to comment.