Separate translation and alignment into different assemblies by pmachapman · Pull Request #1011 · sillsdev/serval

pmachapman · 2026-07-27T04:54:44Z

Fixes #924.

There was some quite tight coupling in Serval.Machine.Shared with the build job runners, which I managed via abstraction rather than duplication, as I wanted to maintain a hierarchy where Serval.Machine.Translation and Serval.Machine.WordAlignment depend on Serval.Machine.Shared, rather than the other way around.

The downside of this approach is that LocalBuildJobRunner and ClearMLBuildJobRunner are each instantiated and run for TranslationEngine and WordAlignmentEngine. I think this should be OK, but if there is a requirement for just one global queue in LocalBuildJobRunner or ClearMLBuildJobRunner that I didn't pick up on, I will need a different approach.

I also took the opportunity to prune some unused code I noticed, clean up the project dependencies, and to migrate to the new SLNX format which I think is much nicer than the old SLN format (especially given I have added 4 new projects in this PR).

This change is

ddaspit

@ddaspit partially reviewed 18 files and all commit messages, and made 8 comments.
Reviewable status: 18 of 121 files reviewed, 8 unresolved discussions (waiting on Enkidu93 and pmachapman).

src/Machine/src/Serval.Machine.Shared/Configuration/IServalConfiguratorExtensions.cs line 19 at r1 (raw file):

        services.Configure<ServiceOptions>(configuration.GetSection(ServiceOptions.Key));
        services.Configure<SharedFileOptions>(configuration.GetSection(SharedFileOptions.Key));
        services.Configure<SmtTransferEngineOptions>(configuration.GetSection(SmtTransferEngineOptions.Key));

This should be moved to AddMachineTranslation.

src/Machine/src/Serval.Machine.Shared/Configuration/IServalConfiguratorExtensions.cs line 20 at r1 (raw file):

        services.Configure<SharedFileOptions>(configuration.GetSection(SharedFileOptions.Key));
        services.Configure<SmtTransferEngineOptions>(configuration.GetSection(SmtTransferEngineOptions.Key));
        services.Configure<StatisticalEngineOptions>(configuration.GetSection(StatisticalEngineOptions.Key));

This should be moved to AddMachineWordAlignment.

src/Machine/src/Serval.Machine.Shared/Configuration/IServalConfiguratorExtensions.cs line 81 at r1 (raw file):

    private static IServalConfigurator AddHealthChecks(this IServalConfigurator configurator)
    {
        var smtTransferEngineOptions = new SmtTransferEngineOptions();

The health checks should be moved to the appropriate assemblies.

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

    private readonly Dictionary<string, ProgressStatus> _curBuildStatus = new();

    private readonly IReadOnlyDictionary<EngineType, string> _queuePerEngineType =

The queues should be filtered according to the correct domain. I would suggest adding an extension method that converts the engine type to the correct engine group, i.e. x.EngineType.ToEngineGroup(). The extension method can be used wherever this conversion is needed. You will probably need to create new classes that inherit from this class to set the engine group.

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

    };

    private readonly Dictionary<EngineGroup, Channel<string>> _jobChannels = new()

Do we still need both?

src/Machine/test/Serval.Machine.Translation.Tests/Services/ClearMLMonitorServiceTests.cs line 4 at r1 (raw file):

[TestFixture]
public class ClearMLMonitorServiceTests

This is very similar to the same test fixture in the word alignment assembly. Can they be shared in some way?

src/Machine/test/Serval.Machine.Translation.Tests/Services/PreprocessBuildJobTests.cs line 4 at r1 (raw file):

[TestFixture]
public class PreprocessBuildJobTests

This is very similar to the same test fixture in the word alignment assembly. Can they be shared in some way?

src/Machine/test/Serval.Machine.Translation.Tests/Services/SmtTransferEngineServiceTests.cs line 315 at r1 (raw file):

            services.AddScoped(_ => _deferredBuildJobService!);
            services.AddKeyedSingleton(EngineGroup.Translation, (_, _) => PlatformService);
            services.AddKeyedSingleton(EngineGroup.WordAlignment, (_, _) => Substitute.For<IPlatformService>());

I don't think this is needed.

ddaspit · 2026-07-27T20:17:34Z

@pmachapman Now that you have a clear idea of how the code is structured with translation and word alignment separated, how do you feel about this change? Do you think it is worth doing? Do you think the separation would make the code more maintainable?

pmachapman

@pmachapman Now that you have a clear idea of how the code is structured with translation and word alignment separated, how do you feel about this change?

@ddaspit I thought this change would be easier than it turned out to be. That said, it is done now, and as a part of it, some boundary issues have been cleaned up (which were what made this change harder than I thought).

The downside is the code is probably more inefficient because this are being twice, i.e. we no longer have just one ClearMLMonitorService running, but two. If we want the most efficient code route, we probably want to blur the boundaries more. I personally think the clearer boundaries are worth the trade off, as before I began I thought there were clearer boundaries between Translation Engine and Word Alignment Engine than I discovered.

Do you think it is worth doing?

Yes, as I think it will simplify the future removal of SMT, and it helps keep the boundary between Translation and Word Alignment Engines clearer. Whether that is a clear boundary we want to keep is an architectural question, and will ultimately decide the merit of this PR.

Do you think the separation would make the code more maintainable?

Yes, I think so, due to the clearer code boundaries.

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

src/Machine/src/Serval.Machine.Shared/Configuration/IServalConfiguratorExtensions.cs line 19 at r1 (raw file):