Fix corpus e2e tests #509

johnml1135 · 2024-10-11T19:34:20Z

A few updates - this should resolve all issues.

This change is

codecov-commenter · 2024-10-11T20:19:57Z

Codecov Report

Attention: Patch coverage is 75.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 56.67%. Comparing base (261dde1) to head (a913230).

Files with missing lines	Patch %	Lines
...hared/Services/ServalTranslationEngineServiceV1.cs	0.00%	9 Missing ⚠️
...l/src/Serval.Translation/Services/EngineService.cs	86.11%	1 Missing and 4 partials ⚠️
...rval.Machine.Shared/Services/PreprocessBuildJob.cs	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #509      +/-   ##
==========================================
+ Coverage   56.62%   56.67%   +0.04%     
==========================================
  Files         299      299              
  Lines       15592    15625      +33     
  Branches     2149     2154       +5     
==========================================
+ Hits         8829     8855      +26     
- Misses       6109     6114       +5     
- Partials      654      656       +2

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

Enkidu93

Thank you for doing this, John! Seems like you got to familiarize yourself with the new code just like you wanted 😆

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit and @johnml1135)

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

    }

    private static IEnumerable<Row> GetPretranslateCorpusNoTarget(ITextCorpus[] srcCorpora)

Seems like this method and AlignPretranslateRows could be merged into a single method. That might allow the logic in WriteDataFiles to be simpler. Just a thought - not necessary.

johnml1135 · 2024-10-11T21:00:08Z

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Seems like this method and AlignPretranslateRows could be merged into a single method. That might allow the logic in WriteDataFiles to be simpler. Just a thought - not necessary.

I wanted to, but the TextRow and ParallelTextRow are incompatible. Slicing it up looked like a bit too much for me. I could possibly make a another class, etc., but I think it is cleaner just to have two functions. Check it out yourself - row.SourceText.Length > 0 vs row.Segment.Count > 0 and !row.IsTargetRangeStart && row.IsTargetInRange vs !row.IsRangeStart && row.IsInRange.

johnml1135 · 2024-10-12T00:10:44Z

Also, Machine will need to be re-released to handle the infinite loop. The E2E tests are failing because of it.

ddaspit

Reviewed 11 of 11 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Enkidu93 and @johnml1135)

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

Previously, johnml1135 (John Lambert) wrote…

I wanted to, but the TextRow and ParallelTextRow are incompatible. Slicing it up looked like a bit too much for me. I could possibly make a another class, etc., but I think it is cleaner just to have two functions. Check it out yourself - row.SourceText.Length > 0 vs row.Segment.Count > 0 and !row.IsTargetRangeStart && row.IsTargetInRange vs !row.IsRangeStart && row.IsInRange.

I think that if you pass in an empty target text corpus to the AlignPretranslateCorpus, then it will work for this case.

src/Machine/src/Serval.Machine.Shared/Services/ServalTranslationEngineServiceV1.cs line 308 at r2 (raw file):

            PretranslateTextIds = pretranslateFilter == FilterChoice.TextIds ? pretranslateTextIds : null
        };
        if (source.PretranslateAll)

GetFilterChoice would be the appropriate place to perform this check. At least, that was where we use to perform this check.

src/Serval/src/Serval.Translation/Services/EngineService.cs line 604 at r2 (raw file):

            new() { Language = source.TargetLanguage, Files = { source.TargetFiles.Select(Map) } };

        if (trainingCorpus == null || (trainingCorpus.TextIds is null && trainingCorpus.ScriptureRange is null))

Might as well use trainingCorpus is null here to make it consistent.

src/Serval/src/Serval.Translation/Services/EngineService.cs line 645 at r2 (raw file):

        }
        if (
            pretranslateCorpus == null

See might comment above.

johnml1135 · 2024-10-15T17:51:23Z

src/Machine/src/Serval.Machine.Shared/Services/ServalTranslationEngineServiceV1.cs line 308 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

GetFilterChoice would be the appropriate place to perform this check. At least, that was where we use to perform this check.

I see now - done.

johnml1135 · 2024-10-15T18:16:57Z

src/Serval/src/Serval.Translation/Services/EngineService.cs line 604 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Might as well use trainingCorpus is null here to make it consistent.

done - trying to add IDE0041 to editorconfig, but it is not catching the errors. Skipping for now.

johnml1135 · 2024-10-15T18:17:48Z

src/Serval/src/Serval.Translation/Services/EngineService.cs line 645 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

See might comment above.

done

johnml1135 · 2024-10-15T18:26:27Z

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

Previously, ddaspit (Damien Daspit) wrote…

I think that if you pass in an empty target text corpus to the AlignPretranslateCorpus, then it will work for this case.

Done.

Put "TrainOnAll" and "PretranslateAll" back in Don't do https redirection Fixes for E2E testing Add E2E test Fix echo

ddaspit

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

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

                targetCorpora.Length > 0
                    ? targetCorpora[0].TextCorpus
                    : new DictionaryTextCorpus(new List<MemoryText>());

Passing in an empty list shouldn't be necessary.

johnml1135 · 2024-10-15T18:44:05Z

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

Previously, ddaspit (Damien Daspit) wrote…

Passing in an empty list shouldn't be necessary.

The fields are nullable in Machine - different version of dotnet...

ddaspit

Reviewed all commit messages.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @johnml1135)

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

Previously, johnml1135 (John Lambert) wrote…

The fields are nullable in Machine - different version of dotnet...

I meant that you don't need to pass an empty list into the DictionaryTextCorpus constructor. I think this change will break things.

johnml1135 · 2024-10-15T19:03:53Z

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

Previously, ddaspit (Damien Daspit) wrote…

I meant that you don't need to pass an empty list into the DictionaryTextCorpus constructor. I think this change will break things.

Yes, it does: System.NullReferenceException: Object reference not set to an instance of an object.
machine-job-cntr | at SIL.Machine.Corpora.ParallelTextCorpus.GetRows(IEnumerable1 textIds)+MoveNext() in /home/johnml1135/repos/machine/src/SIL.Machine/Corpora/ParallelTextCorpus.cs:line 39 machine-job-cntr | at System.Linq.Enumerable.SelectManySingleSelectorIterator2.MoveNext()
machine-job-cntr | at Serval.Machine.Shared.Services.PreprocessBuildJob.AlignPretranslateCorpus(ITextCorpus[] srcCorpora, ITextCorpus trgCorpus)+MoveNext() in /home/johnml1135/repos/serval/src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs:line 424

johnml1135 · 2024-10-15T19:05:40Z

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

Previously, johnml1135 (John Lambert) wrote…

Yes, it does: System.NullReferenceException: Object reference not set to an instance of an object.
machine-job-cntr | at SIL.Machine.Corpora.ParallelTextCorpus.GetRows(IEnumerable1 textIds)+MoveNext() in /home/johnml1135/repos/machine/src/SIL.Machine/Corpora/ParallelTextCorpus.cs:line 39 machine-job-cntr | at System.Linq.Enumerable.SelectManySingleSelectorIterator2.MoveNext()
machine-job-cntr | at Serval.Machine.Shared.Services.PreprocessBuildJob.AlignPretranslateCorpus(ITextCorpus[] srcCorpora, ITextCorpus trgCorpus)+MoveNext() in /home/johnml1135/repos/serval/src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs:line 424

Done

ddaspit

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

johnml1135 requested review from ddaspit and Enkidu93 October 11, 2024 19:34

Enkidu93 approved these changes Oct 11, 2024

View reviewed changes

ddaspit requested changes Oct 14, 2024

View reviewed changes

Pretranslate even if there isn't a target corpus.

6c95566

Put "TrainOnAll" and "PretranslateAll" back in Don't do https redirection Fixes for E2E testing Add E2E test Fix echo

johnml1135 force-pushed the Fix_corpus_e2e_tests branch from 7ec02cf to 6c95566 Compare October 15, 2024 18:27

ddaspit reviewed Oct 15, 2024

View reviewed changes

reviewer comments.

a913230

ddaspit reviewed Oct 15, 2024

View reviewed changes

I see now

29c03fd

ddaspit approved these changes Oct 15, 2024

View reviewed changes

johnml1135 merged commit 30697ae into main Oct 15, 2024
2 checks passed

johnml1135 deleted the Fix_corpus_e2e_tests branch October 24, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix corpus e2e tests #509

Fix corpus e2e tests #509

johnml1135 commented Oct 11, 2024 •

edited by ddaspit

Loading

codecov-commenter commented Oct 11, 2024 •

edited

Loading

Enkidu93 left a comment

johnml1135 commented Oct 11, 2024

johnml1135 commented Oct 12, 2024

ddaspit left a comment

johnml1135 commented Oct 15, 2024

johnml1135 commented Oct 15, 2024

johnml1135 commented Oct 15, 2024

johnml1135 commented Oct 15, 2024

ddaspit left a comment

johnml1135 commented Oct 15, 2024

ddaspit left a comment

johnml1135 commented Oct 15, 2024

johnml1135 commented Oct 15, 2024

ddaspit left a comment

Fix corpus e2e tests #509

Fix corpus e2e tests #509

Conversation

johnml1135 commented Oct 11, 2024 • edited by ddaspit Loading

codecov-commenter commented Oct 11, 2024 • edited Loading

Codecov Report

Enkidu93 left a comment

Choose a reason for hiding this comment

johnml1135 commented Oct 11, 2024

johnml1135 commented Oct 12, 2024

ddaspit left a comment

Choose a reason for hiding this comment

johnml1135 commented Oct 15, 2024

johnml1135 commented Oct 15, 2024

johnml1135 commented Oct 15, 2024

johnml1135 commented Oct 15, 2024

ddaspit left a comment

Choose a reason for hiding this comment

johnml1135 commented Oct 15, 2024

ddaspit left a comment

Choose a reason for hiding this comment

johnml1135 commented Oct 15, 2024

johnml1135 commented Oct 15, 2024

ddaspit left a comment

Choose a reason for hiding this comment

johnml1135 commented Oct 11, 2024 •

edited by ddaspit

Loading

codecov-commenter commented Oct 11, 2024 •

edited

Loading