Better error messages for #62

[johnml1135]

This change is 

[ddaspit]

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

---

machine/translation/huggingface/hugging_face_nmt_engine.py line 52 at r1 (raw file):

                raise ValueError(
                    f"'{src_lang}' is not a valid language code. This error can happen when "
                    + "there is no matching training data and the language code is not in the NLLB-200."

The error message is not appropriate in this context. It references "training data", which is specific to the workflow of a build job. It also references NLLB, but this class is not specific to NLLB. There are two options:

1. Update the message here to give a little more detail, i.e. The specified model does not support the language code '{src_lang}'. or something similar.
2. Catch the error in HuggingFaceNmtModelFactory and throw a new exception with a more detailed message. In that case, we will need to raise a custom error here, so that we can avoid hiding other ValueError instances.

[mshannon-sil]

I am in favor of the first option, since saying that "The specified model does not support the language code '{src_lang}'." is clearer terminology and implicitly directs the user to adjust the model to support the language code, rather than saying that a particular language code "is not valid" and having the user potentially think the issue is with the language code itself.

[johnml1135]

machine/translation/huggingface/hugging_face_nmt_engine.py line 52 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The error message is not appropriate in this context. It references "training data", which is specific to the workflow of a build job. It also references NLLB, but this class is not specific to NLLB. There are two options:

1. Update the message here to give a little more detail, i.e. The specified model does not support the language code '{src_lang}'. or something similar.
2. Catch the error in HuggingFaceNmtModelFactory and throw a new exception with a more detailed message. In that case, we will need to raise a custom error here, so that we can avoid hiding other ValueError instances.

Option 1 is good enough. The error is being raised in production because of another issue that is being fixed now.

[johnml1135]

Done.

[codecov-commenter]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d307a1b) 86.62% compared to head (2143397) 86.62%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
...translation/huggingface/hugging_face_nmt_engine.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #63   +/-   ##
=======================================
  Coverage   86.62%   86.62%           
=======================================
  Files         223      223           
  Lines       13376    13376           
=======================================
  Hits        11587    11587           
  Misses       1789     1789           

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

[ddaspit]

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

---

machine/translation/huggingface/hugging_face_nmt_engine.py line 57 at r2 (raw file):

                and tgt_lang not in additional_special_tokens
            ):
                raise ValueError(f"The specified model does not support the language code '{src_lang}'")

This should be {tgt_lang}.

[johnml1135]

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

---

machine/translation/huggingface/hugging_face_nmt_engine.py line 57 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This should be {tgt_lang}.

Done.

[ddaspit]

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