Build summary gprc

[mudiagaobrikisil]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 68.85246% with 19 lines in your changes missing coverage. Please review.

Project coverage is 63.01%. Comparing base (779cb72) to head (cae9984).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...l.Machine.Shared/Services/ServalPlatformService.cs	0.00%	11 Missing ⚠️
...red/Services/ServalPlatformOutboxMessageHandler.cs	0.00%	5 Missing ⚠️
...taAccess/src/SIL.DataAccess/MemoryUpdateBuilder.cs	78.57%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #541      +/-   ##
==========================================
+ Coverage   62.96%   63.01%   +0.05%     
==========================================
  Files         279      279              
  Lines       13979    14037      +58     
  Branches     1814     1817       +3     
==========================================
+ Hits         8802     8846      +44     
- Misses       4551     4567      +16     
+ Partials      626      624       -2     

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

[johnml1135]

src/Machine/src/Serval.Machine.Shared/Models/Build.cs line 32 at r1 (raw file):

    public required BuildStage Stage { get; init; }
    public string? Options { get; set; }
    public Dictionary<string, int>? Statistics { get; set; }

@ddaspit - what do you think of the name "Statistics"? It is trying to capture any types of information created throughout the build and inference process, including (int the future) possibly warnings or other context.

[johnml1135]

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

        }

        // here's where to add more data to send

This comment needs to be removed (or addressed).

[johnml1135]

src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 1241 at r1 (raw file):

    }

    // private async Task<(int TrainCount, int PretranslateCount)> GetBuildCountsFromLogAsync(

All commented out code should be removed.

[johnml1135]

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

    public string? DeploymentVersion { get; init; }
    public Dictionary<string, string> Statistics { get; init; } =
        new Dictionary<string, string>() { { "trainCount", "0" } };

I would initialize the statistics to an empty array.

[johnml1135]

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

        TranslationBuild build = await _helperClient.TranslationEnginesClient.GetCurrentBuildAsync(engineId);
        Assert.That(build.Statistics, Is.Not.Null);
        Assert.That(build.Statistics, Contains.Key("trainCount"));

I think that a unit test it likely not needed.

[johnml1135]

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

Previously, johnml1135 (John Lambert) wrote…

I think that a unit test it likely not needed.

But an E2E test is needed - add to an existing E2E test such as NmtBatch and verify that the build statistics is properly created.

[johnml1135]

src/Serval/src/Serval.Client/Client.g.cs line 4552 at r1 (raw file):

        /// <returns>The build summary</returns>
        /// <exception cref="ServalApiException">A server side error occurred.</exception>
        System.Threading.Tasks.Task<TranslationBuildSummary> GetBuildSummaryAsync(string id, string buildId, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));

See comment in controller.

[johnml1135]

src/Serval/src/Serval.Translation/Models/TranslationBuildSummary.cs line 7 at r1 (raw file):

    public int LinesTrainedOn { get; set; }
    public int LinesPretranslated { get; set; }
    public TimeSpan TotalBuildTime { get; set; }

This data structure is not needed.

[johnml1135]

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

    public int? QueueDepth { get; init; }
    public JobState State { get; init; } = JobState.Pending;
    public DateTime DateStarted { get; init; } = DateTime.UtcNow;

This can be fine - it allows the users to calculated the total time between creation and date finished. Still, I would name this "Date created".

[johnml1135]

src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 1198 at r1 (raw file):

    }

    /// <summary>

This is actually not needed. The build summary should be present when just normally getting the build.

To do this, you will need to update BuildDto and the map function that maps the fields in the internal "Build" data structure to the BuildDto "data transfer object".

[johnml1135]

Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mudiagaobrikisil)

[johnml1135]

The comments still need to be resolved.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mudiagaobrikisil)

[mudiagaobrikisil]

Hi John, Will update the PR for this today and have a call with you when you are available. Thanks.

…

On Mon, 25 Nov 2024, 10:45 pm John Lambert, ***@***.***> wrote: ***@***.**** requested changes on this pull request. The comments still need to be resolved. *Reviewable <https://reviewable.io/reviews/sillsdev/serval/541#-:-OC_H6dE7kEQq8mN5l6n:bhzceys>* status: all files reviewed, 9 unresolved discussions (waiting on @mudiagaobrikisil <https://github.com/mudiagaobrikisil>) — Reply to this email directly, view it on GitHub <#541 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BLIZ45LJF5ST6G7JW4DYWVL2COK55AVCNFSM6AAAAABR67I2LGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINJZGY4TONBWGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[johnml1135]

src/Machine/src/Serval.Machine.Shared/Models/Build.cs line 32 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

@ddaspit - what do you think of the name "Statistics"? It is trying to capture any types of information created throughout the build and inference process, including (int the future) possibly warnings or other context.

runData?

[mudiagaobrikisil]

Reviewable status: 6 of 17 files reviewed, 9 unresolved discussions (waiting on @johnml1135)

---

src/Serval/src/Serval.Client/Client.g.cs line 4552 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

See comment in controller.

Done.

---

src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 1198 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

This is actually not needed. The build summary should be present when just normally getting the build.

To do this, you will need to update BuildDto and the map function that maps the fields in the internal "Build" data structure to the BuildDto "data transfer object".

Done.

---

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

Previously, johnml1135 (John Lambert) wrote…

I would initialize the statistics to an empty array.

Done.

---

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

Previously, johnml1135 (John Lambert) wrote…

This comment needs to be removed (or addressed).

Done.

---

src/Serval/src/Serval.Translation/Models/TranslationBuildSummary.cs line 7 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

This data structure is not needed.

Done.

[johnml1135]

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 279 at r3 (raw file):

        }

        var newStatistics = new Dictionary<string, string>();

Why was this change made? I am concerned that it will not accurately update parameters, but rather drop existing ones. Some thing you can do is make a unit test for this to ensure that the following happens:
Starting data:

• A: Init
• B: Init
  Updated data:
• B: Update
• C: Update
  Final data:
• A: Init
• B: Update
• C: Update

[johnml1135]

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

Previously, johnml1135 (John Lambert) wrote…

But an E2E test is needed - add to an existing E2E test such as NmtBatch and verify that the build statistics is properly created.

Correction: this is an E2E test (I read it wrong). All statistics fields and values should be confirmed (both the train and pretranslate count).

[johnml1135]

Updates needed:

• E2E test as per the conversation
• The merging Statistics logic looks off. I also think that a unit test could be written. If you don't know how to approach it, we can talk.
• @ddaspit - what do you think of the name "Statistics"? I am inclined to use RunData - or something that can also include things like warnings or other data gathered during the running of the build/job.

[johnml1135]

Reviewed 10 of 11 files at r3, all commit messages.
Reviewable status: 16 of 17 files reviewed, 3 unresolved discussions (waiting on @mudiagaobrikisil)

[ddaspit]

Reviewed 6 of 15 files at r1, 11 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @johnml1135 and @mudiagaobrikisil)

---

src/Machine/src/Serval.Machine.Shared/Models/Build.cs line 32 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

runData?

Maybe ExecutionData?

---

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

        string engineId,
        string buildId,
        IDictionary<string, string> statistics,

I would make this a IReadOnlyDictionary.

---

src/Serval/src/Serval.Translation/Models/Build.cs line 20 at r3 (raw file):

    public IReadOnlyDictionary<string, object>? Options { get; init; }
    public string? DeploymentVersion { get; init; }
    public Dictionary<string, string>[] Statistics { get; init; } = [];

I'm not sure I understand why this is an array of dictionaries. This should be a single dictionary that gets updated with the most recent values.

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 287 at r3 (raw file):

        var updatedStatistics = build.Statistics.Concat(new[] { newStatistics }).ToArray();

        await _builds.UpdateAsync(

The update needs to be atomic or we will face race conditions. We should merge newStatistics into the existing statistics of the model. We will need to add a new "merge" operator to the IUpdateBuilder interface to support this.

[mudiagaobrikisil]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ddaspit and @johnml1135)

---

src/Serval/src/Serval.Translation/Models/Build.cs line 20 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I'm not sure I understand why this is an array of dictionaries. This should be a single dictionary that gets updated with the most recent values.

Just to be sure. Will it keep track of the previous statistics?

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 287 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The update needs to be atomic or we will face race conditions. We should merge newStatistics into the existing statistics of the model. We will need to add a new "merge" operator to the IUpdateBuilder interface to support this.

Noted

[johnml1135]

src/Machine/src/Serval.Machine.Shared/Models/Build.cs line 32 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Maybe ExecutionData?

I like it.

[johnml1135]

src/Machine/src/Serval.Machine.Shared/Models/Build.cs line 32 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

I like it.

Please change all instances of "Statistics" to "ExecutionData".

[johnml1135]

Reviewed 1 of 11 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ddaspit and @mudiagaobrikisil)

[mudiagaobrikisil]

Done

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ddaspit and @johnml1135)

---

src/Machine/src/Serval.Machine.Shared/Models/Build.cs line 32 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Please change all instances of "Statistics" to "ExecutionData".

Done

---

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

Previously, ddaspit (Damien Daspit) wrote…

I would make this a IReadOnlyDictionary.

Done

---

src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 1241 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

All commented out code should be removed.

Done

---

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

Previously, johnml1135 (John Lambert) wrote…

This can be fine - it allows the users to calculated the total time between creation and date finished. Still, I would name this "Date created".

Done

---

src/Serval/src/Serval.Translation/Models/Build.cs line 20 at r3 (raw file):

Previously, mudiagaobrikisil wrote…

Just to be sure. Will it keep track of the previous statistics?

Done

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 279 at r3 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Why was this change made? I am concerned that it will not accurately update parameters, but rather drop existing ones. Some thing you can do is make a unit test for this to ensure that the following happens:
Starting data:

• A: Init
• B: Init
  Updated data:
• B: Update
• C: Update
  Final data:
• A: Init
• B: Update
• C: Update

E2E test done but need a little help with knowing how to track the init and final values in the unit tests, since the ExecutionData is initialized to an empty dictionary

---

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

Previously, johnml1135 (John Lambert) wrote…

Correction: this is an E2E test (I read it wrong). All statistics fields and values should be confirmed (both the train and pretranslate count).

Done

[ddaspit]

Reviewed 5 of 13 files at r4, 2 of 4 files at r5, 9 of 14 files at r6, 1 of 3 files at r7, 2 of 3 files at r8, 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @johnml1135 and @mudiagaobrikisil)

---

samples/ApiExample/ApiExample.csproj line 24 at r6 (raw file):

Previously, mudiagaobrikisil wrote…

On the rebased and squashed commits, it no longer shows that those files changed

It looks like the BOM is getting stripped out of all of the csproj files. I believe that Visual Studio adds them by default for csproj files. We don't really need them. We could strip them from all csproj files to make it consistent.

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 287 at r3 (raw file):

Previously, johnml1135 (John Lambert) wrote…

I told Mudi to get the plain implementation in now and I (or he) could work on updating the DataAccess layer after the other changes are in.

I thought about this a bit more and realized that we don't need a new operator to do this. This update call should do what we want:

await _builds.UpdateAsync(
    b => b.Id == request.BuildId,
    u =>
    {
        foreach (KeyValuePair<string, string> entry in request.ExecutionData)
            u.Set(b => b.ExecutionData[entry.Key], entry.Value);
    },
    cancellationToken: context.CancellationToken
);

---

src/Serval/test/Serval.Translation.Tests/Services/PlatformServiceTests.cs line 126 at r10 (raw file):

        Assert.That(build.ExecutionData, Is.Not.Null);

        var executionData = build.ExecutionData!;

The null-forgiving operator shouldn't be necessary, since you check that it is not null with the assert.

[johnml1135]

samples/ApiExample/ApiExample.csproj line 24 at r6 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

It looks like the BOM is getting stripped out of all of the csproj files. I believe that Visual Studio adds them by default for csproj files. We don't really need them. We could strip them from all csproj files to make it consistent.

As long as we know what changed - stripping the BOM out is fine by me.

[johnml1135]

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 287 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I thought about this a bit more and realized that we don't need a new operator to do this. This update call should do what we want:

await _builds.UpdateAsync(
    b => b.Id == request.BuildId,
    u =>
    {
        foreach (KeyValuePair<string, string> entry in request.ExecutionData)
            u.Set(b => b.ExecutionData[entry.Key], entry.Value);
    },
    cancellationToken: context.CancellationToken
);

If it works, that looks like a fine solution to me.

[johnml1135]

This pull request addresses #558.

[mudiagaobrikisil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ddaspit and @johnml1135)

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 287 at r3 (raw file):

Previously, johnml1135 (John Lambert) wrote…

If it works, that looks like a fine solution to me.

Done

---

src/Serval/test/Serval.Translation.Tests/Services/PlatformServiceTests.cs line 126 at r10 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The null-forgiving operator shouldn't be necessary, since you check that it is not null with the assert.

Done

[ddaspit]

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135 and @mudiagaobrikisil)

[johnml1135]

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mudiagaobrikisil)

[johnml1135]

src/Serval/src/Serval.Translation/Configuration/IMongoDataAccessConfiguratorExtensions.cs line 36 at r12 (raw file):

                await c.UpdateManyAsync(
                    Builders<Build>.Filter.Exists(b => b.ExecutionData, false),
                    Builders<Build>.Update.Set(b => b.ExecutionData, new Dictionary<string, string>())

@ddaspit - what do you think of this - updating the database.

[johnml1135]

There is still a failing test - ###  Serval.Translation.Services.PlatformServiceTests. This needs to be addressed.

[mudiagaobrikisil]

I have addressed it. I will make a push later on

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

[ddaspit]

Reviewed 6 of 6 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnml1135 and @mudiagaobrikisil)

---

src/Serval/src/Serval.Translation/Configuration/IMongoDataAccessConfiguratorExtensions.cs line 36 at r12 (raw file):

Previously, johnml1135 (John Lambert) wrote…

@ddaspit - what do you think of this - updating the database.

ExecutionData should be marked as an optional property or we need a migration like this. If we are going to continue to add migrations, then we should implement more full-fledged migration support with model versions, etc.

[johnml1135]

src/Serval/src/Serval.Translation/Configuration/IMongoDataAccessConfiguratorExtensions.cs line 36 at r12 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

ExecutionData should be marked as an optional property or we need a migration like this. If we are going to continue to add migrations, then we should implement more full-fledged migration support with model versions, etc.

Do we want to do another "one off" for this? I believe it will crash if this is not done.

[johnml1135]

Reviewed 6 of 6 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

[johnml1135]

src/Serval/src/Serval.Translation/Configuration/IMongoDataAccessConfiguratorExtensions.cs line 36 at r12 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Do we want to do another "one off" for this? I believe it will crash if this is not done.

@ddaspit - does this edit meet your expectations?

[ddaspit]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnml1135 and @mudiagaobrikisil)

---

src/Serval/src/Serval.Translation/Configuration/IMongoDataAccessConfiguratorExtensions.cs line 36 at r12 (raw file):

Previously, johnml1135 (John Lambert) wrote…

@ddaspit - does this edit meet your expectations?

I'm not sure we need this. We should be able to mark ExecutionData as optional and get it to work.

---

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 288 at r13 (raw file):

        await _builds.UpdateAsync(
            b => b.Id == request.BuildId,
            u => u.Set(b => b.ExecutionData, updatedExecutionData),

Why was this changed back?

[johnml1135]

src/Serval/src/Serval.Translation/Configuration/IMongoDataAccessConfiguratorExtensions.cs line 36 at r12 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I'm not sure we need this. We should be able to mark ExecutionData as optional and get it to work.

Do we really want it as optional? There will always be execution data (even if it is blank). If it is nullable, the update routine that you wrote before becomes more complicated - I didn't find a good way to initialize the field only if it was null (in a single transaction).

[johnml1135]

src/Serval/src/Serval.Translation/Services/TranslationPlatformServiceV1.cs line 288 at r13 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Why was this changed back?

The bug was in MemoryUpdateBuilder. When it was resolved, the correct implementation could be put in.

[johnml1135]

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

[johnml1135]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ddaspit)

[Enkidu93]

Reviewed 2 of 4 files at r5, 6 of 14 files at r6, 1 of 3 files at r7, 2 of 3 files at r10, 1 of 2 files at r11, 6 of 6 files at r12, 2 of 2 files at r14, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ddaspit)

[ddaspit]

Reviewed 2 of 2 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mudiagaobrikisil)