Serval-side mixed source support #497

Enkidu93 · 2024-09-23T20:56:52Z

THIS IS A DRAFT

There are still tests to add, refactoring to do, and documentation to add as well as a relatively small change to the Machine repo to allow it to handle multiple target files appropriately, but I think you can see the idea. I'll set this aside until I have your feedback and either your OK to wrap things up or your instructions to jump ship and use a different API design. That being said, I may work on the change to machine since I believe that will be needed regardless.

This change is

codecov-commenter · 2024-09-23T21:12:15Z

Codecov Report

Attention: Patch coverage is 69.94144% with 462 lines in your changes missing coverage. Please review.

Project coverage is 56.62%. Comparing base (4382410) to head (7eef883).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
.../Serval.DataFiles/Controllers/CorporaController.cs	0.00%	81 Missing ⚠️
...l.Machine.Shared/Services/ClearMLMonitorService.cs	0.00%	64 Missing ⚠️
...l/src/Serval.Translation/Services/EngineService.cs	78.27%	43 Missing and 10 partials ⚠️
...c/Serval.Machine.Shared/Services/ClearMLService.cs	4.87%	39 Missing ⚠️
...lation/Controllers/TranslationEnginesController.cs	86.80%	26 Missing and 12 partials ⚠️
...hared/Services/ServalTranslationEngineServiceV1.cs	0.00%	24 Missing ⚠️
...ine.Shared/Services/DistributedReaderWriterLock.cs	81.73%	12 Missing and 7 partials ⚠️
...viceToolkit/src/SIL.ServiceToolkit/Utils/TaskEx.cs	45.16%	16 Missing and 1 partial ⚠️
.../Serval.Machine.Shared/Services/BuildJobService.cs	69.23%	10 Missing and 2 partials ⚠️
...rval.Machine.Shared/Services/TimeoutInterceptor.cs	0.00%	10 Missing ⚠️
... and 41 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #497      +/-   ##
==========================================
+ Coverage   56.59%   56.62%   +0.02%     
==========================================
  Files         275      299      +24     
  Lines       14174    15592    +1418     
  Branches     1908     2149     +241     
==========================================
+ Hits         8022     8829     +807     
- Misses       5563     6109     +546     
- Partials      589      654      +65

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

Enkidu93 · 2024-09-24T15:33:39Z

Note: I'm realizing I'm accidentally mixing across corpus pairs (easy to fix) but in case someone gets to looking at this before I make that change, I'm aware.

Enkidu93 · 2024-09-30T19:41:30Z

@ddaspit I pushed a proof-of-concept. If you look at the PreprocessBuildJob, I wrote a second WriteDataFilesAsync function that's public with the updated logic. I tested it in the PreprocessBuildJobTests. Let me know if this looks like the right direction, and I'll pursue it/clean it up/fix the grpc data models etc.

ddaspit · 2024-09-30T22:41:00Z

I took a quick look. This looks like a good direction. You should continue down this path.

… should be written 'TODO') Note: With this new implementation, text marked for pretranslation will be pretranslated even if there's target text for the same verses. I believe this is not an issue (and maybe the previous implementation was a mistake) given the USFM source text preference options.

ddaspit

I'm not finished reviewing yet, but here is the first batch of suggestions. So far this is looking really good.

Reviewed 38 of 44 files at r1, 6 of 8 files at r2, 12 of 25 files at r4, all commit messages.
Reviewable status: 52 of 65 files reviewed, 18 unresolved discussions (waiting on @Enkidu93)

src/Serval/src/Serval.DataFiles/Configuration/IMongoDataAccessConfiguratorExtensions.cs line 27 at r4 (raw file):

    }

    public static IMongoDataAccessConfigurator AddCorporaRepository(this IMongoDataAccessConfigurator configurator)

This can be merged into the AddDataFilesRepositories.

src/Serval/src/Serval.DataFiles/Configuration/IServalBuilderExtensions.cs line 17 at r4 (raw file):

    }

    public static IServalBuilder AddCorpora(this IServalBuilder builder)

This can be merged into the AddDataFiles method.

src/Serval/src/Serval.Translation/Contracts/ParallelCorpusDto.cs line 0 at r4 (raw file):
I don't believe this file is used anywhere.

src/Serval/src/Serval.Translation/Contracts/ParallelCorpusFilterConfigDto.cs line 5 at r4 (raw file):

public record ParallelCorpusFilterConfigDto
{
    public required string CorpusRef { get; init; }

This should be named CorpusId.

src/Serval/src/Serval.Translation/Contracts/PretranslateCorpusConfigDto.cs line 13 at r4 (raw file):

    public string? ParallelCorpusId { get; init; }
    public IReadOnlyList<ParallelCorpusFilterConfigDto>? SourceFilters { get; init; }
    public IReadOnlyList<ParallelCorpusFilterConfigDto>? TargetFilters { get; init; }

The TargetFilters property isn't necessary.

src/Serval/src/Serval.Translation/Contracts/PretranslateCorpusDto.cs line 13 at r4 (raw file):

    public ResourceLinkDto? ParallelCorpus { get; init; }
    public IReadOnlyList<ParallelCorpusFilterDto>? SourceFilters { get; init; }
    public IReadOnlyList<ParallelCorpusFilterDto>? TargetFilters { get; init; }

The TargetFilters property isn't necessary.

src/Serval/src/Serval.Translation/Contracts/TranslationParallelCorpusConfigDto.cs line 10 at r4 (raw file):

    public string? Name { get; init; }

    public required IReadOnlyList<string> SourceCorporaRefs { get; init; } = new List<string>();

I would rename this to SourceCorpusIds.

src/Serval/src/Serval.Translation/Contracts/TranslationParallelCorpusDto.cs line 9 at r4 (raw file):

    public required ResourceLinkDto Engine { get; init; }

    public required IReadOnlyList<TranslationMonolingualCorpusDto> SourceCorpora { get; init; } =

This should be a list of ResourceLinkDtos.

src/Serval/src/Serval.Translation/Contracts/TranslationParallelCorpusDto.cs line 10 at r4 (raw file):

    public required IReadOnlyList<TranslationMonolingualCorpusDto> SourceCorpora { get; init; } =
        new List<TranslationMonolingualCorpusDto>();

For required properties with init setters, it isn't necessary to supply a default value. It will just result in an unnecessary list allocation.

src/Serval/src/Serval.Translation/Contracts/TranslationParallelCorpusUpdateDto.cs line 7 at r4 (raw file):

public record TranslationParallelCorpusUpdateConfigDto : IValidatableObject
{
    public IReadOnlyList<string>? SourceCorpusRefs { get; init; }

I would rename this to SourceCorpusIds.

src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 558 at r4 (raw file):

        await AuthorizeAsync(engine);
        Corpus corpus = await MapAsync(getDataFileClient, idGenerator.GenerateId(), corpusConfig, cancellationToken);
        await _engineService.AddCorpusAsync(id, corpus, cancellationToken);

Should this be adding a ParallelCorpus?

src/Serval/src/Serval.Translation/Models/PretranslateCorpus.cs line 11 at r4 (raw file):

    public string? ParallelCorpusRef { get; set; }
    public IReadOnlyList<ParallelCorpusFilter>? SourceFilters { get; set; }
    public IReadOnlyList<ParallelCorpusFilter>? TargetFilters { get; set; } //TODO is this needed?

We don't need this.

src/Serval/src/Serval.Translation/Services/EngineService.cs line 419 at r4 (raw file):

    }

    public Task AddParallelCorpus(

This should be named AddParallelCorpusAsync.

src/Serval/src/Serval.Translation/Services/EngineService.cs line 425 at r4 (raw file):

    )
    {
        return Entities.UpdateAsync(

Do we want to disallow adding both a ParallelCorpus and a Corpus to the same engine? I didn't see a check for this anywhere.

src/Serval/src/Serval.Translation/Services/EngineService.cs line 597 at r4 (raw file):

    private V1.ParallelCorpus Map(Corpus source, TrainingCorpus? trainingCorpus, PretranslateCorpus? pretranslateCorpus)
    {
        var sourceFiles = source.SourceFiles.Select(Map);

Please specify the type when it isn't explicit elsewhere in the line.

src/Serval/src/Serval.Translation/Services/EngineService.cs line 599 at r4 (raw file):

        var sourceFiles = source.SourceFiles.Select(Map);
        var targetFiles = source.TargetFiles.Select(Map);
        var sourceCorpus = new V1.MonolingualCorpus

I know our codebase is inconsistent about where the type is specified when creating a new object instance, but I've been trying to shift the codebase to specify the type on the left-hand side instead of using var whenever I change anything.

src/Serval/src/Serval.Translation/Services/EngineService.cs line 731 at r4 (raw file):

    )
    {
        var trainOnChapters =

This is probably a bit too much to put on one line of code. Can you refactor this to use an if statement?

src/Serval/src/Serval.Translation/Services/EngineService.cs line 745 at r4 (raw file):

                : null;

        var pretranslateChapters =

See my comment above.

ddaspit

Reviewed 13 of 25 files at r4.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @Enkidu93)

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 134 at r4 (raw file):

                .ToArray();
            ITextCorpus[] sourceTrainingCorpora = sourceCorpora
                .Select(sc =>

This is really hard for me to follow. It would help readability if we merge the two Select calls into one and break up the complex conditional in the second Select into functions. There seems to be some overlap between the logic in the training corpora and the pretranslate corpora that could be shared by refactoring into functions.

src/Machine/test/Serval.Machine.Shared.Tests/Services/PreprocessBuildJobTests.cs line 811 at r4 (raw file):

        }

        public static ParallelCorpus TextFileCorpus(

You can move these static methods into the test fixture class.

Enkidu93

Great! Thank you! Sorry it's taken so long.

Reviewable status: 48 of 65 files reviewed, 20 unresolved discussions (waiting on @ddaspit and @johnml1135)

src/Machine/test/Serval.Machine.Shared.Tests/Services/PreprocessBuildJobTests.cs line 811 at r4 (raw file):