Train on everything if train_on is unspecified; add line in docs

[Enkidu93]

This change is 

[ddaspit]

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

---

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

                                corpus.TrainOnTextIds.Add(trainingCorpus.TextIds);
                        }
                        else if (trainOn?.Count == 0)

We need to check if trainOn is null here.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
src/Serval.Client/Client.g.cs	57.26% <ø> (ø)
...lation/Controllers/TranslationEnginesController.cs	82.85% <ø> (ø)
src/Serval.Translation/Services/EngineService.cs	89.76% <100.00%> (+0.14%)	⬆️

📢 Thoughts on this report? Let us know!

[ddaspit]

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

---

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

Previously, ddaspit (Damien Daspit) wrote…

We need to check if trainOn is null here.

If train_on is specified but empty, then I don't think we should train on anything. This could be a way to indicate that fine tuning is not desired.

[Enkidu93]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @johnml1135)

---

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

Previously, ddaspit (Damien Daspit) wrote…

If train_on is specified but empty, then I don't think we should train on anything. This could be a way to indicate that fine tuning is not desired.

Like this?

[ddaspit]

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

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Like this?

We only want to train on everything if train_on is not specified, so it should be:

else if (trainOn is null)
{
    corpus.TrainOnAll = true;
}

[Enkidu93]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @johnml1135)

---

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

Previously, ddaspit (Damien Daspit) wrote…

We only want to train on everything if train_on is not specified, so it should be:

else if (trainOn is null)
{
    corpus.TrainOnAll = true;
}

Oh, yes, I misread. Done.

[ddaspit]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

[johnml1135]

Has this been tested at least once?

[johnml1135]

Reviewed 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

[Enkidu93]

Has this been tested at least once?

Yes, it has. But I'm glad you asked because I reran the e2e tests and had to fix a small set-up element in the helper to make them all pass given the change to training on all when null not when empty - thank you.

[ddaspit]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

[johnml1135]

Is the comment clear then? If the field train_on is not present, all will be used but if it is present but empty, none will be used. Is that correct? That appears to be what the code says.

[johnml1135]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

[Enkidu93]

Is the comment clear then? If the field train_on is not present, all will be used but if it is present but empty, none will be used. Is that correct? That appears to be what the code says.

I think it's clear. If folks have issues, we can flesh it out. Strictly, the comment is correct.