Separate progress for fine-tuning and inferencing

[pmachapman]

Resolves #477

---

This change is 

[Enkidu93]

Looks good! Thank you, Peter!

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ddaspit)

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 1 at r1 (raw file):

using Google.Protobuf.WellKnownTypes;

Is there a reason we shouldn't move this to the global usings, @ddaspit? It looks like you're the one who put this line here.

---

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

using Serval.Translation.V1;
using Phase = Serval.Translation.V1.Phase;

It looks like that in some files, we're fully qualifying the class names and in other places, we've created aliases like this - @ddaspit, is that OK? Would you prefer we do one or the other?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 81.87919% with 27 lines in your changes missing coverage. Please review.

Project coverage is 66.24%. Comparing base (640860e) to head (2ec0e01).

Files with missing lines	Patch %	Lines
...hared/Services/ServalTranslationPlatformService.cs	0.00%	10 Missing ⚠️
...red/Services/ServalWordAlignmentPlatformService.cs	0.00%	10 Missing ⚠️
...l.Machine.Shared/Services/ClearMLMonitorService.cs	92.85%	1 Missing and 1 partial ⚠️
...ine/src/Serval.Machine.Shared/Models/BuildPhase.cs	75.00%	1 Missing ⚠️
src/Serval/src/Serval.Shared/Contracts/PhaseDto.cs	75.00%	1 Missing ⚠️
src/Serval/src/Serval.Shared/Models/BuildPhase.cs	75.00%	1 Missing ⚠️
...anslation/Services/TranslationPlatformServiceV1.cs	94.11%	0 Missing and 1 partial ⚠️
...ignment/Services/WordAlignmentPlatformServiceV1.cs	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
+ Coverage   66.15%   66.24%   +0.08%     
==========================================
  Files         360      363       +3     
  Lines       19252    19372     +120     
  Branches     2473     2484      +11     
==========================================
+ Hits        12737    12833      +96     
- Misses       5610     5631      +21     
- Partials      905      908       +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]

Reviewed 18 of 20 files at r1, 2 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Enkidu93)

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

It looks like that in some files, we're fully qualifying the class names and in other places, we've created aliases like this - @ddaspit, is that OK? Would you prefer we do one or the other?

I don't have a problem with using aliases. It would be good to be consistent, but we can do that over time.

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 1 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Is there a reason we shouldn't move this to the global usings, @ddaspit? It looks like you're the one who put this line here.

I don't remember. Maybe there are types that have conflicting names.

---

src/ServiceToolkit/src/SIL.ServiceToolkit/Models/BuildPhase.cs line 9 at r3 (raw file):

}

public record BuildPhase

I would prefer not to share models across assemblies using the service toolkit. If a shared service needs a model, then it is fine. Can you define these separately in each assembly?

[Enkidu93]

Reviewed 2 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ddaspit and @pmachapman)

---

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

Previously, ddaspit (Damien Daspit) wrote…

I don't have a problem with using aliases. It would be good to be consistent, but we can do that over time.

Alright, sounds good

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 1 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't remember. Maybe there are types that have conflicting names.

I don't think there are any errors building if it's moved.

[ddaspit]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93 and @pmachapman)

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 1 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I don't think there are any errors building if it's moved.

Then I don't have a problem with making this a global using.

[ddaspit]

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93 and @pmachapman)

[pmachapman]

Reviewable status: 14 of 26 files reviewed, 2 unresolved discussions (waiting on @ddaspit and @Enkidu93)

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 1 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Then I don't have a problem with making this a global using.

Implementing this as a global using results in the following error occurring multiple times: 'Enum' is an ambiguous reference between 'Google.Protobuf.WellKnownTypes.Enum' and 'System.Enum'.

Given that using Google.Protobuf.WellKnownTypes; is only in one file per project, except for Serval.Machine.Shared where 3 files have this using, I don't think it should be made a global using.

---

src/ServiceToolkit/src/SIL.ServiceToolkit/Models/BuildPhase.cs line 9 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I would prefer not to share models across assemblies using the service toolkit. If a shared service needs a model, then it is fine. Can you define these separately in each assembly?

Done. I have moved it to Serval.Machine.Shared.Models, and duplicated into Serval.Shared.Models. @ddaspit Let me know if these were the wrong places!

[ddaspit]

Reviewed 13 of 13 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Enkidu93)

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 1 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Implementing this as a global using results in the following error occurring multiple times: 'Enum' is an ambiguous reference between 'Google.Protobuf.WellKnownTypes.Enum' and 'System.Enum'.

Given that using Google.Protobuf.WellKnownTypes; is only in one file per project, except for Serval.Machine.Shared where 3 files have this using, I don't think it should be made a global using.

I'm fine with leaving this as a local using.

---

src/Serval/src/Serval.Translation/Contracts/TranslationBuildDto.cs line 13 at r5 (raw file):

    public IReadOnlyList<PretranslateCorpusDto>? Pretranslate { get; init; }
    public required int Step { get; init; }
    public double? PercentCompleted { get; init; }

This PR is an opportunity to fix the PercentCompleted property. The name causes confusion. I would like to add a new Progress property that holds the same value as PercentCompleted and deprecate the PercentCompleted property. @pmachapman How would you feel about making that change in this PR? Or would you prefer that we do that in another PR?

---

src/Machine/src/Serval.Machine.Shared/Models/BuildPhase.cs line 13 at r5 (raw file):

    public required BuildPhaseStage Stage { get; init; }
    public required int Step { get; init; }
    public required int StepCount { get; init; }

I think the StepCount property should be optional. There might be cases where a build phase doesn't have a known step count. For example, training a model using early stopping would not have a known step count.

[pmachapman]

Reviewable status: 15 of 26 files reviewed, 3 unresolved discussions (waiting on @ddaspit and @Enkidu93)

---

src/Serval/src/Serval.Translation/Contracts/TranslationBuildDto.cs line 13 at r5 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This PR is an opportunity to fix the PercentCompleted property. The name causes confusion. I would like to add a new Progress property that holds the same value as PercentCompleted and deprecate the PercentCompleted property. @pmachapman How would you feel about making that change in this PR? Or would you prefer that we do that in another PR?

I'd need to talk more about it to get an idea of what you are envisaging. Let's discuss tomorrow briefly, if you are free.

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 1 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I'm fine with leaving this as a local using.

Done.

---

src/Machine/src/Serval.Machine.Shared/Models/BuildPhase.cs line 13 at r5 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I think the StepCount property should be optional. There might be cases where a build phase doesn't have a known step count. For example, training a model using early stopping would not have a known step count.

I did contemplate making it nullable for the reason you outlined, but settled on its current form with 0 as the default "unknown value" value, as 0 would never be a step count and I felt the code is a little messier with the nullable/optional support.

I have made the change, but can revert it if you think the non-null implementation is preferable.

[Enkidu93]

Reviewable status: 15 of 26 files reviewed, 2 unresolved discussions (waiting on @ddaspit)

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 1 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Done.

That's strange that I didn't get that error before. Sounds good!

[Enkidu93]

Reviewable status: 15 of 26 files reviewed, 2 unresolved discussions (waiting on @ddaspit)

[pmachapman]

Reviewable status: 15 of 26 files reviewed, 2 unresolved discussions (waiting on @dd, @ddaspit, and @Enkidu93)

---

src/Serval/src/Serval.Translation/Contracts/TranslationBuildDto.cs line 13 at r5 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I'd need to talk more about it to get an idea of what you are envisaging. Let's discuss tomorrow briefly, if you are free.

@ddaspit I have made two additional commits - one that just renames percentCompleted to progress on the API, and a second more comprehensive change across all of the layers.

I have not updated the C# Machine or machine.py to use the new nomenclature (see ProgressStatus in Machine)

[ddaspit]

Reviewed 3 of 11 files at r6, 20 of 20 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

---

src/Serval/src/Serval.Translation/Contracts/TranslationBuildDto.cs line 13 at r5 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

@ddaspit I have made two additional commits - one that just renames percentCompleted to progress on the API, and a second more comprehensive change across all of the layers.

I have not updated the C# Machine or machine.py to use the new nomenclature (see ProgressStatus in Machine)

The comprehensive change wasn't strictly necessary, but it does result in cleaner/clearer code, so I'm happy with the comprehensive change.

---

src/Machine/src/Serval.Machine.Shared/Models/BuildPhase.cs line 13 at r5 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I did contemplate making it nullable for the reason you outlined, but settled on its current form with 0 as the default "unknown value" value, as 0 would never be a step count and I felt the code is a little messier with the nullable/optional support.

I have made the change, but can revert it if you think the non-null implementation is preferable.

I don't have a problem with using 0 internally. Mainly, I would like the StepCount property on PhaseDto to be nullable, so that it is unambiguous when a stage doesn't have a known number of steps. But what you have done is totally fine, so you don't need to change it again.