Align pretranslations by default

[Enkidu93]

Change requested here.

---

This change is 

[isaac091]

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

[Enkidu93]

How come Isaac's approval isn't enough for me to merge this in 🤔? Surely you have write access, @isaac091?

[ddaspit]

It looks like Isaac just left a comment and didn't actually approve the PR.

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

---

machine/jobs/build_nmt_engine.py line 58 at r1 (raw file):

                raise TypeError(f"Build options could not be parsed: {e}") from e
            SETTINGS.update({model_type: build_options})
            if "align_pretranslations" in build_options:

I don't think we want to remove this. We want a way to disable alignments using build options.

[Enkidu93]

machine/jobs/build_nmt_engine.py line 58 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't think we want to remove this. We want a way to disable alignments using build options.

Do you expect that we'd need to be able to turn this off from the client side though? My thinking was that now SF or some other client could turn off alignments at build time and then when they go to fetch USFM and place markers, it won't work properly, but there won't be visibility as to why. As it stands, this is something we could still turn off from within Serval when we define the machine.py script I believe.

[Enkidu93]

Oh, yes 😆, thank you. I'm so used to = approved.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

[isaac091]

Yeah sorry, I was letting Damien have the final say.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

[ddaspit]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)

---

machine/jobs/build_nmt_engine.py line 58 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Do you expect that we'd need to be able to turn this off from the client side though? My thinking was that now SF or some other client could turn off alignments at build time and then when they go to fetch USFM and place markers, it won't work properly, but there won't be visibility as to why. As it stands, this is something we could still turn off from within Serval when we define the machine.py script I believe.

I was thinking it might be useful to turn it off for testing purposes.

[Enkidu93]

No worries - makes sense.

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

---

machine/jobs/build_nmt_engine.py line 58 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I was thinking it might be useful to turn it off for testing purposes.

OK, that makes sense. Done.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.93%. Comparing base (5579c2d) to head (a1ce450).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #195   +/-   ##
=======================================
  Coverage   88.93%   88.93%           
=======================================
  Files         282      282           
  Lines       17056    17056           
=======================================
  Hits        15169    15169           
  Misses       1887     1887           

☔ 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.

[ddaspit]

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