Properly update model name

[Enkidu93]

Fixes #1032.

---

This change is 

[pmachapman]

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

[ddaspit]

I have some comments for this PR. Don't merge it in yet. Thanks.

[Enkidu93]

Yep! Sorry - I actually meant to request your review on this one - sorry! Thank you!

@Enkidu93 made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Enkidu93).

[ddaspit]

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

---

src/Serval/test/Serval.E2ETests/ServalApiTests.cs line 289 at r1 (raw file):

            },
        ];
        _helperClient.TranslationBuildConfig.Model = "NLLB";

Should this be NLLB600m?

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 105 at r1 (raw file):

        }

        if (buildOptionsJsonNode != null && buildOptionsJsonNode is JsonObject buildOptionsJsonObject)

The build options are the only way that the model is passed to the build job. The options are not updated with the model if options is null.

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 121 at r1 (raw file):

        model ??= Models.Models.Nllb;
        CheckIsValidModel(model);
        await _platformService.UpdateModelAsync(buildId, model, cancellationToken);

Rather than adding a new callback method on the platform service, it would be simpler if we just return the model from this method call. I don't think there is a problem with inserting the Build after calling this method.

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 247 at r1 (raw file):

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

A static dictionary would be a good way to capture these mappings.

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 259 at r1 (raw file):

        {
            "facebook/nllb-200-distilled-1.3B" => Models.Models.Nllb,
            "facebook/nllb-200-distilled-600M" => Models.Models.Nllb600m,

I know this was set in a previous PR, but I feel the values should be nllb, nllb-600m and nllb-testing.

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 269 at r1 (raw file):

        if (model != Models.Models.Nllb && model != Models.Models.Nllb600m && model != Models.Models.NllbTesting)
        {
            throw new ArgumentException($"Unknown model {model}.");

This should throw an InvalidOperationException, so that a 400 status code is returned from the endpoint. You should also check the code paths that call GetFullModelName and GetModelName.

---

src/Machine/src/Serval.Machine.Translation/Services/EchoTranslationEngineService.cs line 87 at r1 (raw file):

            corpora,
            options,
            model,

Echo will never set CurrentBuild.Model. Is that intentional? There is code in EchoPreprocessBuildJob that reads the model.

---

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

        try
        {
            string? currentModel = (await Engines.GetAsync(e => e.EngineId == engineId, cancellationToken))

Rather than doing this extra query, you should be able to just pass through model on every call to StartBuildJobAsync just like we do with the buildOptions parameter.

[Enkidu93]

Please let me know if I missed anything. My work on this got cut short last week and it's always hard to remember exactly where you were with something like this.

@Enkidu93 made 9 comments.
Reviewable status: 9 of 47 files reviewed, 8 unresolved discussions (waiting on ddaspit and pmachapman).

---

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

Previously, ddaspit (Damien Daspit) wrote…

Rather than doing this extra query, you should be able to just pass through model on every call to StartBuildJobAsync just like we do with the buildOptions parameter.

Done. This definitely causes a bit of a ripple effect but like you say, we already do this with the build options.

---

src/Machine/src/Serval.Machine.Translation/Services/EchoTranslationEngineService.cs line 87 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Echo will never set CurrentBuild.Model. Is that intentional? There is code in EchoPreprocessBuildJob that reads the model.

Sorry, yep, my intention was to not do anything with the model name (just leave it null) in echo. I think this is no longer relevant given the other changes. I changed this to make it set the model name to "echo".

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 105 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The build options are the only way that the model is passed to the build job. The options are not updated with the model if options is null.

Done.

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 121 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Rather than adding a new callback method on the platform service, it would be simpler if we just return the model from this method call. I don't think there is a problem with inserting the Build after calling this method.

True, I did think of this. The reason I did not go this direction is 1) it's a little odd for this method to return the model name and 2) the model name is not relevant for SMT and Echo. I've gone ahead and made this change as you've recommended, but if you'd prefer I revert, I can.

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 247 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

A static dictionary would be a good way to capture these mappings.

Done.

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 259 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I know this was set in a previous PR, but I feel the values should be nllb, nllb-600m and nllb-testing.

Done. Yeah, that's nicer.

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 269 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This should throw an InvalidOperationException, so that a 400 status code is returned from the endpoint. You should also check the code paths that call GetFullModelName and GetModelName.

Done...if I'm following the second part correctly.

---

src/Serval/test/Serval.E2ETests/ServalApiTests.cs line 289 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Should this be NLLB600m?

Sorry, yes, done. That was left over from testing.

[ddaspit]

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

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 121 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

True, I did think of this. The reason I did not go this direction is 1) it's a little odd for this method to return the model name and 2) the model name is not relevant for SMT and Echo. I've gone ahead and made this change as you've recommended, but if you'd prefer I revert, I can.

Yeah, it is a little weird. Maybe we could return a contract object that contains the defaults for unset values in a new build.

---

src/Serval/src/Serval.Translation/Features/Engines/StartBuild.cs line 125 at r2 (raw file):

                    .GetEngineService(engine.Type)
                    .StartBuildAsync(engine.Id, build.Id, corpora, buildOptions, build.Model, ct);
                await builds.UpdateAsync(b => b.Id == build.Id, u => u.Set(b => b.Model, model), cancellationToken: ct);

You should be able to insert the build after calling StartBuildAsync, so we don't need the extra update operation.

[Enkidu93]

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

---

src/Serval/src/Serval.Translation/Features/Engines/StartBuild.cs line 125 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You should be able to insert the build after calling StartBuildAsync, so we don't need the extra update operation.

Maybe I'm missing something: How would I get the build id for StartBuildAsync() except from the inserted build?

[ddaspit]

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

---

src/Serval/src/Serval.Translation/Features/Engines/StartBuild.cs line 125 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Maybe I'm missing something: How would I get the build id for StartBuildAsync() except from the inserted build?

You can inject an IIdGenerator service to get a new build id. Just search the codebase to find some examples of how it is used.

[Enkidu93]

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

---

src/Machine/src/Serval.Machine.Translation/Services/NmtEngineService.cs line 121 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Yeah, it is a little weird. Maybe we could return a contract object that contains the defaults for unset values in a new build.

I went ahead and just made a generic start build contract which at the moment just captures the model. Nothing else really seemed appropriate to put there. Happy to move something there if you think it'd make sense.

---

src/Serval/src/Serval.Translation/Features/Engines/StartBuild.cs line 125 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You can inject an IIdGenerator service to get a new build id. Just search the codebase to find some examples of how it is used.

That's a good point. Thanks!

[ddaspit]

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

[pmachapman]

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