Cleanup builds & engines

[Enkidu93]

Fixes #374

---

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 89.41176% with 9 lines in your changes missing coverage. Please review.

Project coverage is 56.76%. Comparing base (261dde1) to head (65c72c4).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...lation/Services/UnitializedEntityCleanupService.cs	81.25%	5 Missing and 1 partial ⚠️
...l/src/Serval.Translation/Services/EngineService.cs	90.90%	1 Missing and 1 partial ⚠️
src/Serval/src/Serval.Translation/Models/Build.cs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #510      +/-   ##
==========================================
+ Coverage   56.62%   56.76%   +0.14%     
==========================================
  Files         299      302       +3     
  Lines       15592    15662      +70     
  Branches     2149     2151       +2     
==========================================
+ Hits         8829     8891      +62     
- Misses       6109     6115       +6     
- Partials      654      656       +2     

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

[ddaspit]

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

---

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

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

I would name this IsCreated or IsInitialized. Since this is a new property, it should be optional.

---

src/Serval/src/Serval.Translation/Models/Engine.cs line 19 at r1 (raw file):

    public double Confidence { get; init; }
    public int CorpusSize { get; init; }
    public bool SuccessfullyCreated { get; init; }

I would name this IsCreated or IsInitialized. Since this is a new property, it should be optional.

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 6 at r1 (raw file):

namespace Serval.Translation.Services;

public class BuildCleanupService(

I wonder if we could find a way to make this class generic. I feel like this will be a normal pattern in Serval. We could create a new interface for these types of models. It would extend IEntity and contain the flag property. If this isn't clear, we can meet to discuss further.

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 25 at r1 (raw file):

    public async Task CheckBuildsAsync(IRepository<Build> builds, CancellationToken cancellationToken)
    {
        IReadOnlyList<Build> allBuilds = await builds.GetAllAsync(cancellationToken);

You should filter the builds in the query.

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 27 at r1 (raw file):

        IReadOnlyList<Build> allBuilds = await builds.GetAllAsync(cancellationToken);
        IEnumerable<Build> notStartedBuilds = allBuilds.Where(b => !b.SuccessfullyStarted);
        await Task.Delay(_timeout, cancellationToken); //Make sure the builds are not midway through starting

Using a DateCreated property would be more deterministic and reliable. It would be a useful property to have anyway.

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 30 at r1 (raw file):

        foreach (
            Build build in await builds.GetAllAsync(
                b => notStartedBuilds.Select(c => c.Id).Contains(b.Id),

I would perform the Select in the original query.

---

src/Serval/src/Serval.Translation/Services/EngineCleanupService.cs line 30 at r1 (raw file):

    )
    {
        IReadOnlyList<Engine> allEngines = await engines.GetAllAsync(cancellationToken);

See my comments for BuildCleanupService.

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 149 at r1 (raw file):

                IsModelPersisted = createResponse.IsModelPersisted
            };
            await Entities.UpdateAsync(

We should update IsModelPersisted in the same call. We shouldn't pass a cancellation token to this call.

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 300 at r1 (raw file):

            }
            await client.StartBuildAsync(request, cancellationToken: cancellationToken);
            await _builds.UpdateAsync(

We shouldn't pass a cancellation token to this call.

[Enkidu93]

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

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 6 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I wonder if we could find a way to make this class generic. I feel like this will be a normal pattern in Serval. We could create a new interface for these types of models. It would extend IEntity and contain the flag property. If this isn't clear, we can meet to discuss further.

No, I understand and I considered doing something like that. Really, the only substantial difference between these classes besides the model they're operating on is how they're deleted (i.e., via the engine service in the EngineCleanupService) but that just be an overridden method. I'll go ahead and do something like that and ping you for review once I'm done.

[Enkidu93]

I know I should be focusing my attention on the other PR, but I had this commit just about ready to push, so I figured I should just get it out there. I'm not so sure about the naming, so let me know if you think there would be better names for the new class and interface etc.

Reviewable status: 1 of 11 files reviewed, 9 unresolved discussions (waiting on @ddaspit and @johnml1135)

---

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

Previously, ddaspit (Damien Daspit) wrote…

I would name this IsCreated or IsInitialized. Since this is a new property, it should be optional.

Done.

---

src/Serval/src/Serval.Translation/Models/Engine.cs line 19 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I would name this IsCreated or IsInitialized. Since this is a new property, it should be optional.

Done.

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 6 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

No, I understand and I considered doing something like that. Really, the only substantial difference between these classes besides the model they're operating on is how they're deleted (i.e., via the engine service in the EngineCleanupService) but that just be an overridden method. I'll go ahead and do something like that and ping you for review once I'm done.

Done.

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 25 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You should filter the builds in the query.

Done.

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 27 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Using a DateCreated property would be more deterministic and reliable. It would be a useful property to have anyway.

Done.

---

src/Serval/src/Serval.Translation/Services/BuildCleanupService.cs line 30 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I would perform the Select in the original query.

Done.

---

src/Serval/src/Serval.Translation/Services/EngineCleanupService.cs line 30 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

See my comments for BuildCleanupService.

Done.

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 149 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We should update IsModelPersisted in the same call. We shouldn't pass a cancellation token to this call.

Like this? I'm not sure I totally understand the point of the updateIsModelPersisted flag. And does it need to be updated outside of the try like it was previously?

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 300 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We shouldn't pass a cancellation token to this call.

Done.

[johnml1135]

src/Serval/src/Serval.Translation/Services/EngineService.cs line 149 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Like this? I'm not sure I totally understand the point of the updateIsModelPersisted flag. And does it need to be updated outside of the try like it was previously?

If IsModelPersisted was specified from the API, it is set properly in the Entities it doesn't need to be updated, otherwise, it does from the Machine level defaults. There is no issue with the change.

[johnml1135]

src/Serval/src/Serval.Translation/Services/UnitializedEntityCleanupService.cs line 47 at r2 (raw file):

    )
    {
        await entities.DeleteAsync(e => e.Id == entity.Id, cancellationToken);

How does this actually do anything? How is it cleaning up the builds (this is not overridden)?

[ddaspit]

In the EngineService and BuildService, we should treat engines and builds that haven't been initialized as if they don't exist.

Reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Enkidu93 and @johnml1135)

---

src/Serval/src/Serval.Translation/Services/EngineCleanupService.cs line 27 at r2 (raw file):

        if (EngineService == null)
            return;
        await EngineService.DeleteAsync(engine.Id, cancellationToken);

I don't think we need to use the service to delete the engine. We can just use the repository. If an engine wasn't initialized properly, then you won't be able to create builds and pretranslations.

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 156 at r2 (raw file):

                    u.Set(e => e.IsModelPersisted, engine.IsModelPersisted);
                },
                cancellationToken: cancellationToken

You shouldn't pass a cancellation token to this call.

---

src/Serval/src/Serval.Translation/Services/UnitializedEntityCleanupService.cs line 27 at r2 (raw file):

    {
        var now = DateTime.UtcNow;
        IEnumerable<T> uninitializedEntities = (await entities.GetAllAsync(cancellationToken)).Where(b =>

The Where class should be included in the database query, i.e. entities.GetAllAsync(e => e.IsInitalized != null && !e.IsInitialized && e.DateCreated != null && e.DateCreated > expired).

[johnml1135]

src/Serval/src/Serval.Translation/Services/UnitializedEntityCleanupService.cs line 47 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

How does this actually do anything? How is it cleaning up the builds (this is not overridden)?

I misunderstood - now I see what is going on.

[johnml1135]

Reviewed 1 of 9 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Enkidu93)

[Enkidu93]

Done. Let me know if you think there's a cleaner way to do this. It'd be nice if I could make them inherit from some kind of InitializableEntityServiceBase maybe but - alas - no multiple inheritance.

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

---

src/Serval/src/Serval.Translation/Services/EngineCleanupService.cs line 27 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't think we need to use the service to delete the engine. We can just use the repository. If an engine wasn't initialized properly, then you won't be able to create builds and pretranslations.

Done. Good point.

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 149 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

If IsModelPersisted was specified from the API, it is set properly in the Entities it doesn't need to be updated, otherwise, it does from the Machine level defaults. There is no issue with the change.

OK, great

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 156 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You shouldn't pass a cancellation token to this call.

Oops. Done!

---

src/Serval/src/Serval.Translation/Services/UnitializedEntityCleanupService.cs line 27 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The Where class should be included in the database query, i.e. entities.GetAllAsync(e => e.IsInitalized != null && !e.IsInitialized && e.DateCreated != null && e.DateCreated > expired).

Done.

[ddaspit]

I think this is fine. You could add InitializableEntityServiceBase and OwnedInitializableEntityServiceBase, and duplicate the code between the two. I don't love it, but it would work. The other option is to make everything an initializable entity. For entities that don't need the two-stage initialization, you could just set IsInitialized to true when the entity is created.

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Enkidu93)

---

src/Serval/src/Serval.Translation/Services/BuildService.cs line 3 at r3 (raw file):

namespace Serval.Translation.Services;

public class BuildService(IRepository<Build> builds) : EntityServiceBase<Build>(builds), IBuildService

Do you need to override GetAsync?

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 6 at r3 (raw file):

namespace Serval.Translation.Services;

public class EngineService(

Don't you need to override GetAllAsync?

---

src/Serval/src/Serval.Translation/Services/EngineService.cs line 365 at r3 (raw file):

    }

    public Task AddCorpusAsync(string engineId, Models.Corpus corpus, CancellationToken cancellationToken = default)

All of the corpus methods need to check the flag.

---

src/Serval/src/Serval.Translation/Services/UnitializedEntityCleanupService.cs line 0 at r3 (raw file):
This class should go in Serval.Shared.

---

src/Serval/src/Serval.Translation/Services/UnitializedEntityCleanupService.cs line 27 at r3 (raw file):

    {
        var now = DateTime.UtcNow;
        IEnumerable<T> uninitializedEntities = await entities.GetAllAsync(

Would an index be appropriate for this query?

[johnml1135]

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

[johnml1135]

Doesn't this also resolve #468?