Create target quote convention per build

[Enkidu93]

Fixes #849.

---

This change is 

[codecov-commenter]

Codecov Report

❌ Patch coverage is 78.12500% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.46%. Comparing base (b73ab6b) to head (83c76aa).

Files with missing lines	Patch %	Lines
...hared/Services/ServalTranslationPlatformService.cs	0.00%	6 Missing ⚠️
...sumers/TranslationUpdateQuoteConventionConsumer.cs	0.00%	5 Missing ⚠️
....Shared/Configuration/IMachineBuilderExtensions.cs	0.00%	1 Missing ⚠️
...l.Machine.Shared/Services/NmtPreprocessBuildJob.cs	88.88%	0 Missing and 1 partial ⚠️
...slation/Controllers/TranslationBuildsController.cs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #861      +/-   ##
==========================================
+ Coverage   66.42%   66.46%   +0.03%     
==========================================
  Files         382      382              
  Lines       20782    20791       +9     
  Branches     2717     2714       -3     
==========================================
+ Hits        13805    13819      +14     
+ Misses       6007     6005       -2     
+ Partials      970      967       -3     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ddaspit]

@ddaspit partially reviewed 18 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93).

---

src/Serval/src/Serval.Translation/Models/Build.cs line 25 at r1 (raw file):

    public DateTime? DateCompleted { get; set; }
    public IReadOnlyList<BuildPhase>? Phases { get; init; }
    public IReadOnlyList<ParallelCorpusAnalysis>? Analysis { get; init; }

What about existing builds? Do we need to migrate them to use the new property? Can we remove this property?

---

src/Serval/src/Serval.Translation/Services/PretranslationService.cs line 301 at r1 (raw file):

        if (
            quoteNormalizationBehavior == PretranslationNormalizationBehavior.Denormalized
            && build.TargetQuoteConvention is not null

You can use string.IsNullOrEmpty() here.

[Enkidu93]

@Enkidu93 made 2 comments.
Reviewable status: 15 of 18 files reviewed, 2 unresolved discussions (waiting on @ddaspit).

---

src/Serval/src/Serval.Translation/Models/Build.cs line 25 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

What about existing builds? Do we need to migrate them to use the new property? Can we remove this property?

Yes, sorry - I wanted to pick your brain about that. I just pushed a commit with one of the options: Keep the property for now and populate the per-build quote convention using the analyses for the older builds.

Another option would be to migrate these builds to use the new per-build quote convention. My only concern with this option is this: If, say, we release to QA and do this migration and then realize we'd like to do things differently for some reason, we will have lost some information in the process, right? I'm trying to remember how we've handled this before. We've had migrations where we've added properties, but have we removed properties like this?

---

src/Serval/src/Serval.Translation/Services/PretranslationService.cs line 301 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You can use string.IsNullOrEmpty() here.

Done.

[Enkidu93]

@Enkidu93 made 1 comment.
Reviewable status: 15 of 18 files reviewed, 2 unresolved discussions (waiting on @ddaspit).

---

src/Serval/src/Serval.Translation/Models/Build.cs line 25 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Yes, sorry - I wanted to pick your brain about that. I just pushed a commit with one of the options: Keep the property for now and populate the per-build quote convention using the analyses for the older builds.

Another option would be to migrate these builds to use the new per-build quote convention. My only concern with this option is this: If, say, we release to QA and do this migration and then realize we'd like to do things differently for some reason, we will have lost some information in the process, right? I'm trying to remember how we've handled this before. We've had migrations where we've added properties, but have we removed properties like this?

Nevermind - I'm not sure what I was thinking 🤦‍♂️. We don't need to delete any data from the DB, just remove the property from the model, right? So there's no problem. I guess I should just migrate the model itself then.

[ddaspit]

@ddaspit reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93).

---

src/Serval/src/Serval.Translation/Models/Build.cs line 25 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Nevermind - I'm not sure what I was thinking 🤦‍♂️. We don't need to delete any data from the DB, just remove the property from the model, right? So there's no problem. I guess I should just migrate the model itself then.

Yes, we don't need to actual delete the Analysis property. If we remove it from the model, we will need to perform a migration that sets the new TargetQuoteConvention property based on the old Analysis property.

[Enkidu93]

@Enkidu93 made 3 comments.
Reviewable status: 12 of 21 files reviewed, 3 unresolved discussions (waiting on @ddaspit).

---

src/Serval/src/Serval.Translation/Models/Build.cs line 25 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Yes, we don't need to actual delete the Analysis property. If we remove it from the model, we will need to perform a migration that sets the new TargetQuoteConvention property based on the old Analysis property.

Yep, done! I'm going to test the migration on int-qa. I'll let you know when that is complete.

---

src/Serval/src/Serval.Translation/Controllers/TranslationControllerBase.cs line 41 at r4 (raw file):

            ExecutionData = Map(source.ExecutionData),
            Phases = source.Phases?.Select(Map).ToList(),
            Analysis = Map(engine, targetQuoteConvention).ToList(),

Unfortunately, I realized that this is more complicated then I thought. Because TrainOn could be null, there could conceivably be analyses for all parallel corpora. Instead of adding logic here to only add those dummy analyses in cases where TrainOn is null, I thought it was simpler just to add a dummy analysis for all of them. Sadly, that means I need access to the engine's list of parallel corpora (unless you have an alternative 🤔).

---

src/Serval/src/Serval.Translation/Controllers/TranslationBuildsController.cs line 45 at r4 (raw file):

            builds = await _buildService.GetAllCreatedAfterAsync(Owner, createdAfter, cancellationToken);
        }
        return await Task.WhenAll(

Is this the right way to do this? I know it works but will this cause things like EntityNotFoundExceptions not to surface properly and get caught in the filter?

[ddaspit]

@ddaspit reviewed 9 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93).

---

src/Serval/src/Serval.Translation/Controllers/TranslationControllerBase.cs line 41 at r4 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Unfortunately, I realized that this is more complicated then I thought. Because TrainOn could be null, there could conceivably be analyses for all parallel corpora. Instead of adding logic here to only add those dummy analyses in cases where TrainOn is null, I thought it was simpler just to add a dummy analysis for all of them. Sadly, that means I need access to the engine's list of parallel corpora (unless you have an alternative 🤔).

I really don't want to perform all of these extra queries. I think I would rather put the Analysis property back and add the analysis objects when TargetQuoteConvention is set.

[Enkidu93]

@Enkidu93 made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit).

---

src/Serval/src/Serval.Translation/Controllers/TranslationControllerBase.cs line 41 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I really don't want to perform all of these extra queries. I think I would rather put the Analysis property back and add the analysis objects when TargetQuoteConvention is set.

OK 😆 - can do 🫡.

[Enkidu93]

@Enkidu93 made 1 comment and resolved 1 discussion.
Reviewable status: 16 of 21 files reviewed, all discussions resolved (waiting on @ddaspit).

---

src/Serval/src/Serval.Translation/Controllers/TranslationControllerBase.cs line 41 at r4 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

OK 😆 - can do 🫡.

Done. I believe everything related to that has been reverted.

[ddaspit]

@ddaspit reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93).

---

src/Serval/src/Serval.Translation/Controllers/TranslationControllerBase.cs line 41 at r4 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done. I believe everything related to that has been reverted.

Unless I'm missing something, I think you forgot to add the analysis objects in TranslationPlatformServiceV1.

[Enkidu93]

@Enkidu93 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit).

---

src/Serval/src/Serval.Translation/Controllers/TranslationControllerBase.cs line 41 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Unless I'm missing something, I think you forgot to add the analysis objects in TranslationPlatformServiceV1.

I think I just misunderstood what you requested, but you're right: What I have now will not be backwards compatible with how SF is checking canDenormalize so we should also add the dummy analyses back to the model 👍. I'll go ahead and do that.

[Enkidu93]

@Enkidu93 made 1 comment.
Reviewable status: 19 of 21 files reviewed, 1 unresolved discussion (waiting on @ddaspit).

---

src/Serval/src/Serval.Translation/Controllers/TranslationControllerBase.cs line 41 at r4 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I think I just misunderstood what you requested, but you're right: What I have now will not be backwards compatible with how SF is checking canDenormalize so we should also add the dummy analyses back to the model 👍. I'll go ahead and do that.

Done.

[ddaspit]

@ddaspit reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93).