SF-2900 Mixed Source: Selecting books from each source #2825

pmachapman · 2024-10-28T18:58:20Z

This PR adds support for parallel corpora in Serval, and backend support for selecting books from each source.

This is a key requirement for the mixed source logic, which is currently behind a feature flag.

The frontend does not yet allow selection of different books for each source.

Blockers for QA

The parallel Corpus database holds onto old Filename when the file is updated serval#547

This change is

codecov · 2024-10-28T19:43:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.75%. Comparing base (9b92c3e) to head (f681783).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2825      +/-   ##
==========================================
+ Coverage   79.19%   79.75%   +0.56%     
==========================================
  Files         533      534       +1     
  Lines       30962    30952      -10     
  Branches     5052     5030      -22     
==========================================
+ Hits        24520    24687     +167     
+ Misses       5668     5511     -157     
+ Partials      774      754      -20

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

kylebuss

Looking good! I can definitely see what you meant when you mentioned waiting on SF-2525 for these changes.

Reviewed 6 of 9 files at r1, 23 of 23 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @pmachapman)

src/SIL.XForge.Scripture/Models/ServalAdditionalTrainingData.cs line 7 at r2 (raw file):

/// <summary>
/// Serval Configuration for Additional Training Data.
/// </summary>

Is there a link or class in Serval for reference to this?

Code quote:

/// </summary>

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 729 at r2 (raw file):

            );
        }

Can you add a comment here to specify Translation section and above (line 702) for Training Section? The code is so similar I thought it was duplicated.

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 394 at r2 (raw file):

        // If there is an alternate source, ensure that writing system and RTL is correct
        if (projectDoc.Data.TranslateConfig.DraftConfig.AlternateSource is not null)

We might consider updating the settings.FullName, as that can also be changed in the Paratext registry.

Code quote:

if (projectDoc.Data.TranslateConfig.DraftConfig.AlternateSource is not null)

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 408 at r2 (raw file):

                        alternateSourceSettings.IsRightToLeft
                    );
                    if (alternateSourceSettings.LanguageTag is not null)

While unlikely, should we verify the primary source and alternate/additional source language tags match and error if they do not?

Code quote:

if (alternateSourceSettings.LanguageTag is not null)

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 587 at r2 (raw file):

    /// <returns>The parallel corpus identifier.</returns>
    /// <remarks>This can be mocked in unit tests.</remarks>
    protected internal virtual async Task<string> CreateOrUpdateParallelCorpusAsync(

Curious to the reasoning behind making this method's virtual?

Code quote:

internal virtual async

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 860 at r2 (raw file):

                }

                // Update the source writing system tag

It might be an unlikely scenario, but I wonder if we may want to check if RTL has changed for the source?

Code quote:

// Update the source writing system tag

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 1162 at r2 (raw file):

                cancellationToken
            );
            bool recreateTranslationEngine = false;

If RTL has changed should we also recreate the translation engine?

Code quote:

recreateTranslationEngine

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 1346 at r2 (raw file):

            // Upload the source texts
            List<ServalCorpusFile> sourceCorpusFiles = [.. additionalTrainingData.CorpusFiles];

Seen the Range operator used a few times with this PR, what are some of the benefits to using this over simply having = additionalTrainingData.CorpusFiles;?

Code quote:

[.. additionalTrainingData.CorpusFiles]

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 1701 at r2 (raw file):

                translationParallelCorpusId
            );
        }

Do we need the if/else here?

I wonder if we can differentiate between NMT and SMT in the few places like this:

string sourceProjectId = preTranslate ? hasAlternateSource
        ? project.TranslateConfig.DraftConfig.AlternateSource.ProjectRef
        : project.TranslateConfig.Source.ProjectRef
        : project.TranslateConfig.Source.ProjectRef;

And then put the alternate/additional sources in a single if(preTranslate) { ...}?

Code quote:

        else
        {
            // Set up the parallel corpus for SMT translation
            string sourceProjectId = project.TranslateConfig.Source.ProjectRef;
            List<ServalCorpusFile> sourceCorpora = [servalCorpusFiles.Single(f => f.ProjectId == sourceProjectId)];
            List<ServalCorpusFile> targetCorpora = [servalCorpusFiles.Single(f => f.ProjectId == project.Id)];
            List<string> sourceCorpusIds = sourceCorpora.Select(f => f.CorpusId).ToList();
            List<string> targetCorpusIds = targetCorpora.Select(f => f.CorpusId).ToList();

            // Get the pre-translate parallel corpus id (might be null)
            translationParallelCorpusId = projectSecret.ServalData.ParallelCorpusIdForSmt;

            // Create or update the pre-translate parallel corpora
            translationParallelCorpusId = await CreateOrUpdateParallelCorpusAsync(
                translationEngineId,
                translationParallelCorpusId,
                name: "SmtTranslation",
                sourceCorpusIds,
                targetCorpusIds,
                cancellationToken
            );

            // Record the corpus sync info
            corporaSyncInfo = RecordServalCorpusSyncInfo(
                corporaSyncInfo,
                sourceCorpora,
                targetCorpora,
                translationParallelCorpusId
            );
        }

pmachapman

Reviewable status: 25 of 30 files reviewed, 8 unresolved discussions (waiting on @kylebuss)

src/SIL.XForge.Scripture/Models/ServalAdditionalTrainingData.cs line 7 at r2 (raw file):