Structured build diagnostics

[Enkidu93]

Fixes #965.

See https://docs.google.com/document/d/1719NBOAx0NRvk8WpcciTqvFPPZDRSK-oZXbrIKUgL20/edit?tab=t.0#heading=h.dsj2hidolq5 for details.

This work so far does not include:

• Adding tests
• Updating documentation - I'm figuring we'll want this in the wiki?
• The correct confidence threshold

---

This change is 

[codecov-commenter]

Codecov Report

❌ Patch coverage is 82.46269% with 141 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.44%. Comparing base (195bf5a) to head (ce24984).

Files with missing lines	Patch %	Lines
...gnment/Services/WordAlignmentPreprocessBuildJob.cs	0.00%	28 Missing ⚠️
....Translation/Services/NmtClearMLBuildJobFactory.cs	0.00%	19 Missing and 1 partial ⚠️
...src/Serval.Translation/Services/PlatformService.cs	84.04%	9 Missing and 6 partials ⚠️
...nslation/Services/TranslationPreprocessBuildJob.cs	55.55%	9 Missing and 3 partials ⚠️
...nt/Services/EchoWordAlignmentPreprocessBuildJob.cs	55.55%	9 Missing and 3 partials ⚠️
...erval/src/Serval.Translation/Services/DtoMapper.cs	9.09%	10 Missing ⚠️
...val/src/Serval.WordAlignment/Services/DtoMapper.cs	16.66%	10 Missing ⚠️
...rval.Machine.Shared/Services/PreprocessBuildJob.cs	95.10%	1 Missing and 6 partials ⚠️
...ine.Translation/Services/EchoPreprocessBuildJob.cs	70.00%	4 Missing and 2 partials ⚠️
src/Serval/src/Serval.Client/Client.g.cs	50.00%	5 Missing ⚠️
... and 8 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1012      +/-   ##
==========================================
+ Coverage   72.00%   72.44%   +0.44%     
==========================================
  Files         386      391       +5     
  Lines       20886    21632     +746     
  Branches     2778     2815      +37     
==========================================
+ Hits        15039    15672     +633     
- Misses       4738     4835      +97     
- Partials     1109     1125      +16     

☔ View full report in Codecov by Harness.
📢 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 reviewed 31 files and all commit messages, and made 19 comments.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on Enkidu93).

---

a discussion (no related file):
Are you planning on adding unit tests? And yes, we will want to add a page for the diagnostics to the wiki page. It would probably be good to have a link to the wiki page in the Swagger docs.

---

src/Serval/src/Serval.Translation/Dtos/ExecutionDataDto.cs line 14 at r1 (raw file):

    [Obsolete]
    public IReadOnlyList<string> Warnings { get; init; } = [];
    public IReadOnlyList<DiagnosticDto> Diagnostics { get; init; } = [];

This should be optional.

---

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

            sourceLanguageTag,
            targetLanguageTag,
            true,

Make the parameter name explicit.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 475 at r1 (raw file):

            .Select(b => new Diagnostic
            {
                Code = "MODEL-004",

I think this is supposed to be MODEL-003.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 479 at r1 (raw file):

                Severity = Models.DiagnosticSeverity.Warn,
                Message =
                    $"The average pretranslation model confidence {b.averageConfidence} in book {b.bookId} is unusually low for the base model {ModelName}",

This confidence should be formatted.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 488 at r1 (raw file):

            });

        if (badBookConfidences.Any())

This deferred LINQ query is getting run multiple times.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 492 at r1 (raw file):

            Build? currentBuild = await _builds.GetAsync(b => b.Id == buildId, cancellationToken);

            await _builds.UpdateAsync(

Can we perform this update and the next in a single Mongo call?

---

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

            sourceLanguageTag,
            targetLanguageTag,
            true,

Make the parameter name explicit.

---

src/Serval/src/Serval.Translation/Dtos/DiagnosticDto.cs line 3 at r1 (raw file):

namespace Serval.Translation.Dtos;

public record DiagnosticDto

We should call this TranslationDiagnosticDto, so the name doesn't collide with the word alignment diagnostic in Serval.Client. ExecutionDataDto should really be renamed too, but that would be a breaking change at this point.

---

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

    }

    protected virtual IReadOnlyList<BuildDiagnostic> GetDiagnostics(

We have a MaxWarnings. We should probably have a MaxDiagnostics.

---

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

                string versificationName,
                IReadOnlyList<UsfmVersificationDiagnosticContract> usfmDiagnostics
            ) in ParallelCorpusService.AnalyzeUsfmVersification(parallelCorpora)

I don't love that we are running AnalyzeUsfmVersification twice for a build. Could we do this once and pass it to GetWarnings and GetDiagnostics or derive the warnings from the diagnostics?

---

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

                        Serval.Shared.Contracts.UsfmVersificationDiagnosticType.InvalidChapter => new BuildDiagnostic
                        {
                            Code = "USFM-001",

This is probably overkill, but I would feel a bit less anxious if the number part of the code was 4 digits.

---

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

        }

        if (projectVersifications.Count > 1)

I don't think this is correct. It is checking the number of projects, not the number of versifications.

---

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

                    Severity = BuildDiagnosticSeverity.Info,
                    Message =
                        $"There are multiple versifications represented among Paratext projects selected for training or inferencing: {string.Join(", ", projectVersifications)}.",

Are you intending to print out the dictionary entries?

---

src/Serval/src/Serval.WordAlignment/Dtos/WordAlignmentExecutionDataDto.cs line 14 at r1 (raw file):

    [Obsolete]
    public IReadOnlyList<string> Warnings { get; init; } = [];
    public IReadOnlyList<DiagnosticDto> Diagnostics { get; init; } = [];

This should be optional.

---

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

    private readonly ILanguageTagService _languageTagService = languageTagService;
    private const string ModelName = "NLLB";
    private const string MinimumTrainCount = "600"; //TODO move to options?

This should be an int and probably be an option.

---

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

                    Data = new Dictionary<string, object>
                    {
                        { "resolvedCode", sourceLanguageTag },

Is this supposed to be resolvedCode instead of sourceLanguageTag?

---

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

                    Data = new Dictionary<string, object>
                    {
                        { "resolvedCode", targetLanguageTag },

Is this supposed to be resolvedCode instead of targetLanguageTag?

---

src/Serval/src/Serval.WordAlignment/Dtos/DiagnosticDto.cs line 3 at r1 (raw file):

namespace Serval.WordAlignment.Dtos;

public record DiagnosticDto

This should be called WordAlignmentDiagnosticDto.

[Enkidu93]

@Enkidu93 made 19 comments.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on ddaspit).

---

a discussion (no related file):

Previously, ddaspit (Damien Daspit) wrote…

Are you planning on adding unit tests? And yes, we will want to add a page for the diagnostics to the wiki page. It would probably be good to have a link to the wiki page in the Swagger docs.

Yep, I don't know if you saw the high-level comment I left when I opened the PR. Those are among the TODOs. (I know sometimes Reviewable seems to bury those top-level initial comments which is frustrating.) I wasn't sure exactly how we want to handle the wiki: I figured once the code and document have both been reviewed, I'll basically copy the doc into the wiki and link it in the StartBuild Swagger docs since there isn't a way, I don't think, to make the wiki update part of this PR per se. I will work on tests though - although I expect those will be a little hairy.

---

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

Previously, ddaspit (Damien Daspit) wrote…

Make the parameter name explicit.

Done

---

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

Previously, ddaspit (Damien Daspit) wrote…

Make the parameter name explicit.

Done.

---

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

Previously, ddaspit (Damien Daspit) wrote…

This should be an int and probably be an option.

OK, I made it an int. In general, how do we want to handle these options? I don't like having the model name in two classes for example. I suppose we could have some kind of diagnostics factory that has these options configured. It could have a method like CreateDiagnostic(string code, Dictionary<string, object> data) and we could validate all the data parameters there. Then we could also more easily test the factory with unit tests and then just confirm that the factory method is being called correctly with a mock in the build job classes. I think it also would make it easier to add new diagnostics. The only tricky part would be that this service would need to be called from both PlatformService (or at least that's the natural place) and the build job classes and potentially other places in the future. I'm not sure where a class like this would live. What do you think?

---

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

Previously, ddaspit (Damien Daspit) wrote…

Is this supposed to be resolvedCode instead of sourceLanguageTag?

Yes, good catch. Something unit tests will catch hopefully 🤪.

---

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

Previously, ddaspit (Damien Daspit) wrote…

Is this supposed to be resolvedCode instead of targetLanguageTag?

Yes, done.

---

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

Previously, ddaspit (Damien Daspit) wrote…

We have a MaxWarnings. We should probably have a MaxDiagnostics.

OK, as long as it's a pretty high value, I think that's fine. I figure we'll want to sort by severity before limiting. The only thing that's a little weird is adding a diagnostic that means there were too many diagnostics 🤔 - I'm not sure I like that. What do you think? Maybe it's fine.

---

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

Previously, ddaspit (Damien Daspit) wrote…

I don't love that we are running AnalyzeUsfmVersification twice for a build. Could we do this once and pass it to GetWarnings and GetDiagnostics or derive the warnings from the diagnostics?

We could. The only reason I did it this way was so that the warnings and diagnostics were completely separate. That way we could strip out the warnings easily when the time comes without affecting the diagnostics and adding the diagnostics wouldn't affect the warnings. But I could just make the warnings a list of all the diagnostic messages.

---

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

Previously, ddaspit (Damien Daspit) wrote…

This is probably overkill, but I would feel a bit less anxious if the number part of the code was 4 digits.

OK 😆, I can do that.

---

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

Previously, ddaspit (Damien Daspit) wrote…

I don't think this is correct. It is checking the number of projects, not the number of versifications.

Yes, sorry, I switched to a map since Nathaniel asked but didn't update this properly. Done.

---

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

Previously, ddaspit (Damien Daspit) wrote…

Are you intending to print out the dictionary entries?

I think we probably ought to. What do you think? I can format them another way explicitly if you prefer.

---

src/Serval/src/Serval.Translation/Dtos/DiagnosticDto.cs line 3 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We should call this TranslationDiagnosticDto, so the name doesn't collide with the word alignment diagnostic in Serval.Client. ExecutionDataDto should really be renamed too, but that would be a breaking change at this point.

Done. Yeah, I noticed that :/.

---

src/Serval/src/Serval.Translation/Dtos/ExecutionDataDto.cs line 14 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This should be optional.

Done.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 475 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I think this is supposed to be MODEL-003.

Yes, done.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 479 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This confidence should be formatted.

Done.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 488 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This deferred LINQ query is getting run multiple times.

Done. Good catch. Just ToList()'d it. Do we have a preference for the new spread syntax where we can just use ToList()? I totally understand using it for concatenation, but I feel like it's a little less readable than, e.g., ToList() it situations like these.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 492 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Can we perform this update and the next in a single Mongo call?

Yep, done.

---

src/Serval/src/Serval.WordAlignment/Dtos/DiagnosticDto.cs line 3 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This should be called WordAlignmentDiagnosticDto.

Done.

---

src/Serval/src/Serval.WordAlignment/Dtos/WordAlignmentExecutionDataDto.cs line 14 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This should be optional.

Done.

[ddaspit]

@ddaspit partially reviewed 15 files and all commit messages, made 7 comments, and resolved 15 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Enkidu93).

---

a discussion (no related file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Yep, I don't know if you saw the high-level comment I left when I opened the PR. Those are among the TODOs. (I know sometimes Reviewable seems to bury those top-level initial comments which is frustrating.) I wasn't sure exactly how we want to handle the wiki: I figured once the code and document have both been reviewed, I'll basically copy the doc into the wiki and link it in the StartBuild Swagger docs since there isn't a way, I don't think, to make the wiki update part of this PR per se. I will work on tests though - although I expect those will be a little hairy.

Yes, that sounds right.

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

OK, I made it an int. In general, how do we want to handle these options? I don't like having the model name in two classes for example. I suppose we could have some kind of diagnostics factory that has these options configured. It could have a method like CreateDiagnostic(string code, Dictionary<string, object> data) and we could validate all the data parameters there. Then we could also more easily test the factory with unit tests and then just confirm that the factory method is being called correctly with a mock in the build job classes. I think it also would make it easier to add new diagnostics. The only tricky part would be that this service would need to be called from both PlatformService (or at least that's the natural place) and the build job classes and potentially other places in the future. I'm not sure where a class like this would live. What do you think?

When you create the diagnostic factory in Serval, make sure that it isn't coupled to the Machine engine implementation in any way.

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

OK, as long as it's a pretty high value, I think that's fine. I figure we'll want to sort by severity before limiting. The only thing that's a little weird is adding a diagnostic that means there were too many diagnostics 🤔 - I'm not sure I like that. What do you think? Maybe it's fine.

I think it is fine. We need some way to communicate that we hit the max. We certainly don't want to create a huge Build that we store in the DB or pass from the API endpoint.

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

We could. The only reason I did it this way was so that the warnings and diagnostics were completely separate. That way we could strip out the warnings easily when the time comes without affecting the diagnostics and adding the diagnostics wouldn't affect the warnings. But I could just make the warnings a list of all the diagnostic messages.

If we generate the warnings from the diagnostics, it should be easy to strip out.

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

I think we probably ought to. What do you think? I can format them another way explicitly if you prefer.

I would prefer that we explicitly formatted them even if it is basically the same as ToString generates for a dictionary entry.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 488 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done. Good catch. Just ToList()'d it. Do we have a preference for the new spread syntax where we can just use ToList()? I totally understand using it for concatenation, but I feel like it's a little less readable than, e.g., ToList() it situations like these.

I prefer ToList, but I also don't like fighting with the suggestions from VS Code. I'm fine with either.

---

src/Serval/src/Serval.WordAlignment/Dtos/WordAlignmentDiagnosticDto.cs line 3 at r2 (raw file):

namespace Serval.WordAlignment.Dtos;

public record WordAlignmentDiagnosticDto

Thinking about this a bit more. I think this should be a shared DTO in Serval.Shared. The corresponding model can also be shared.

[Enkidu93]

@Enkidu93 made 6 comments.
Reviewable status: 11 of 38 files reviewed, 5 unresolved discussions (waiting on ddaspit).

---

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

Previously, ddaspit (Damien Daspit) wrote…

When you create the diagnostic factory in Serval, make sure that it isn't coupled to the Machine engine implementation in any way.

Let me know how this looks. There's still a question of where to move a couple of the options/parameters (the confidence threshold and the model name), but otherwise, I think this is complete. There's a bit of me that thinks the model name should just be a property of the Build. We've made this diagnostics generic entirely because we expect to be adding new models; and if we add new models, I don't think we want the model type just living in Options. I could just put the confidence threshold in the options for the platform service or maybe outsource that check to another class which would make it more extensible if we change how we threshold to something more complex (?).

---

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

Previously, ddaspit (Damien Daspit) wrote…

I think it is fine. We need some way to communicate that we hit the max. We certainly don't want to create a huge Build that we store in the DB or pass from the API endpoint.

OK, done.

---

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

Previously, ddaspit (Damien Daspit) wrote…

If we generate the warnings from the diagnostics, it should be easy to strip out.

Done.

---

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

Previously, ddaspit (Damien Daspit) wrote…

I would prefer that we explicitly formatted them even if it is basically the same as ToString generates for a dictionary entry.

Done.

---

src/Serval/src/Serval.Translation/Services/PlatformService.cs line 488 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I prefer ToList, but I also don't like fighting with the suggestions from VS Code. I'm fine with either.

Yeah, me too. OK.

---

src/Serval/src/Serval.WordAlignment/Dtos/WordAlignmentDiagnosticDto.cs line 3 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Thinking about this a bit more. I think this should be a shared DTO in Serval.Shared. The corresponding model can also be shared.

Done.

[ddaspit]

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

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

OK, done.

The only other option would be to add a flag to ExecutionData that indicates that the max was hit. It might be cleaner. @pmachapman What do you think would be best from Scripture Forge's perspective?

---

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

        }
        return diagnostics
            .Select(d => new BuildDiagnostic

I don't love how we create the Serval contracts, convert them to Machine models, then convert them back to Serval contracts. The right direction would be to create the Machine models, then convert them to the Serval contracts. Or get rid of the Machine models altogether and just pass the Serval contracts.

---

src/Serval/src/Serval.Shared/Services/BuildDiagnosticService.cs line 26 at r3 (raw file):

    private static readonly Dictionary<string, DiagnosticInfo> Diagnostics = new()
    {

I like that this provides a clear spec of the diagnostics along with a way to validate the data field.

[Enkidu93]

@Enkidu93 made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ddaspit and pmachapman).

---

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

Previously, ddaspit (Damien Daspit) wrote…

The only other option would be to add a flag to ExecutionData that indicates that the max was hit. It might be cleaner. @pmachapman What do you think would be best from Scripture Forge's perspective?

Yeah, I'd prefer that. We'll see what Peter thinks.

---

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

Previously, ddaspit (Damien Daspit) wrote…

I don't love how we create the Serval contracts, convert them to Machine models, then convert them back to Serval contracts. The right direction would be to create the Machine models, then convert them to the Serval contracts. Or get rid of the Machine models altogether and just pass the Serval contracts.

Me neither. The problem is that we can't have a Serval service creating Machine models. You could pass the contract models themselves but then that'd require the BuildExecutionData having the contract as the type of one of its properties. My consolation in this back and forth is that we do have the contracts working both directions (e.g. the UsfmVersificationDiagnosticContract goes Serval -> Machine whereas BuildProgressStatusContract goes the other way). This contract is just going both directions: Calculated by Serval, sent to Machine, processed, and then ultimately used to communicate back to Serval. And we have to have this service in Serval.Shared because it is used in both Serval and Machine. I'm not sure what we can do. I suppose we could make to contracts to separate the two ways of communication, but they would be identical.

---

src/Serval/src/Serval.Shared/Services/BuildDiagnosticService.cs line 26 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I like that this provides a clear spec of the diagnostics along with a way to validate the data field.

Me too. I wish that it were strongly typed such that you were forced to supply the correct data types and it'd fail at compile time otherwise, but I don't see a way to do that without making a class for each diagnostic.

[pmachapman]

@pmachapman made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ddaspit).

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Yeah, I'd prefer that. We'll see what Peter thinks.

Either option is fine, I think.

[ddaspit]

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

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Me neither. The problem is that we can't have a Serval service creating Machine models. You could pass the contract models themselves but then that'd require the BuildExecutionData having the contract as the type of one of its properties. My consolation in this back and forth is that we do have the contracts working both directions (e.g. the UsfmVersificationDiagnosticContract goes Serval -> Machine whereas BuildProgressStatusContract goes the other way). This contract is just going both directions: Calculated by Serval, sent to Machine, processed, and then ultimately used to communicate back to Serval. And we have to have this service in Serval.Shared because it is used in both Serval and Machine. I'm not sure what we can do. I suppose we could make to contracts to separate the two ways of communication, but they would be identical.

We should just remove the Diagnostic model and pass the Serval contracts directly. BuildExecutionData.Diagnostics can just use the contract type. That is better than converting back and forth.

[Enkidu93]

@Enkidu93 made 2 comments.
Reviewable status: 22 of 74 files reviewed, 1 unresolved discussion (waiting on ddaspit and pmachapman).

---

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

Previously, pmachapman (Peter Chapman) wrote…

Either option is fine, I think.

I went ahead and switched to using a flag.

---

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

Previously, ddaspit (Damien Daspit) wrote…

We should just remove the Diagnostic model and pass the Serval contracts directly. BuildExecutionData.Diagnostics can just use the contract type. That is better than converting back and forth.

Done.

[Enkidu93]

I've gone ahead and added tests for the diagnostics. I'm realizing in general we need to add some more testing for word alignment now that they are separate, but that's not new with this PR, so if it's OK with you, I'd prefer addressing that separately since this PR has already snowballed quite a bit.

[Enkidu93]

I went ahead and added a link to the wiki. I kept it very short in the Swagger docs since we really don't have any other information regarding things in the execution data. I can elaborate if you think it's necessary but then I should probably elaborate on other things as well. (We could create an issue?). I will work on adding what's in the doc to the wiki, but I don't think that will take long or needs to hold up this PR.

[ddaspit]

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

---

src/Machine/src/Serval.Machine.Translation/Services/ServalTranslationPlatformService.cs line 109 at r5 (raw file):

                Warnings = executionData.Warnings,
                Diagnostics = executionData
                    .Diagnostics?.Select(d => new DiagnosticContract

Unless I'm missing something, I don't think this is still necessary.

---

src/Machine/src/Serval.Machine.WordAlignment/Services/ServalWordAlignmentPlatformService.cs line 109 at r5 (raw file):

                Diagnostics =
                    executionData
                        .Diagnostics?.Select(d => new DiagnosticContract

Unless I'm missing something, I don't think this is still necessary.

---

src/Serval/src/Serval.Translation/Models/BaseModel.cs line 14 at r5 (raw file):

        return model switch
        {
            BaseModel.NLLB => "facebook/nllb-200-distilled-1.3B",

The Machine engine should define this mapping.

---

src/Serval/src/Serval.Translation/Models/Build.cs line 19 at r5 (raw file):

    public DateTime? DateFinished { get; init; }
    public IReadOnlyDictionary<string, object>? Options { get; init; }
    public BaseModel? BaseModel { get; init; }

This should be a string. Defining the base model values in Serval is a bit of a leaky abstraction. The Machine engine should define and validate the value. The values can be defined as string constants.