Migrate to transformers 5

[pmachapman]

This change is 

[codecov-commenter]

Codecov Report

❌ Patch coverage is 83.07692% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.05%. Comparing base (615cd59) to head (fcf9486).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...nslation/huggingface/transformers_compatibility.py	72.50%	22 Missing ⚠️
...tion/huggingface/hugging_face_nmt_model_trainer.py	63.63%	16 Missing ⚠️
...translation/huggingface/hugging_face_nmt_engine.py	94.64%	3 Missing ⚠️
.../translation/huggingface/hugging_face_nmt_model.py	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #363      +/-   ##
==========================================
- Coverage   92.13%   92.05%   -0.08%     
==========================================
  Files         390      394       +4     
  Lines       24644    24844     +200     
==========================================
+ Hits        22705    22871     +166     
- Misses       1939     1973      +34     

☔ View full report in Codecov by Harness.
📢 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.

[Enkidu93]

Adding @mshannon-sil as a reviewer here as well. Thank you for doing this, Peter!

[Enkidu93]

I know this was a lot of work. Thank you, Peter! These kinds of changes always make me nervous. Unfortunately, Mike is no longer doing his BLEU tests (he mentioned SF is picking those up somehow? Not sure what he meant exactly?). Maybe we should begin doing that sort of test ourselves.

@Enkidu93 reviewed 17 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ddaspit and mshannon-sil).

[ddaspit]

Thanks for taking this on. A few things I found while going through it; the batch_prepare_for_model one is the only one I think has to be fixed before merging.

One more that isn't in the diff: samples/machine_translation.ipynb still passes overwrite_output_dir=True, which is gone in v5, so that cell will raise.

[mshannon-sil]

@mshannon-sil reviewed 8 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on pmachapman).

---

.gitignore line 147 at r2 (raw file):

# Ignore custom pyright configuration
pyrightconfig.json

Do we not want a standard pyrightconfig.json for the repository?

---

machine/translation/huggingface/transformers_compatibility.py line 1 at r2 (raw file):

import enum

What are the benefits of recreating the TranslationPipeline class ourselves, now that it's been removed from transformers? Is it just that it's more similar to what was here before? SILNLP no longer imports Pipeline at all from transformers, choosing to create a standaloneSilTranslator class instead. I had Devin review the two approaches, and it seems that using something similar to SILNLP's approach here could cut down on overhead and reduce opportunities for bugs. It also seems to make intuitive sense to me that we shouldn't need to create a whole file for transformers compatibility to replicate transformers 4.x code, if instead we can rearchitect it to feel like more natural transformers 5.x code. But I admit, I don't have a 100% understanding of the pros and cons of both approaches, so feel free to share if there's a production, repo, or other reason why keeping the pipeline makes more sense.

[pmachapman]

One more that isn't in the diff: samples/machine_translation.ipynb still passes overwrite_output_dir=True, which is gone in v5, so that cell will raise.

Done. Thanks!

@pmachapman made 18 comments and resolved 2 discussions.
Reviewable status: 8 of 18 files reviewed, 15 unresolved discussions (waiting on ddaspit, Enkidu93, and mshannon-sil).

---

machine/translation/huggingface/transformers_compatibility.py line 1 at r2 (raw file):

Previously, mshannon-sil wrote…

What are the benefits of recreating the TranslationPipeline class ourselves, now that it's been removed from transformers? Is it just that it's more similar to what was here before? SILNLP no longer imports Pipeline at all from transformers, choosing to create a standaloneSilTranslator class instead. I had Devin review the two approaches, and it seems that using something similar to SILNLP's approach here could cut down on overhead and reduce opportunities for bugs. It also seems to make intuitive sense to me that we shouldn't need to create a whole file for transformers compatibility to replicate transformers 4.x code, if instead we can rearchitect it to feel like more natural transformers 5.x code. But I admit, I don't have a 100% understanding of the pros and cons of both approaches, so feel free to share if there's a production, repo, or other reason why keeping the pipeline makes more sense.

I needed to implement Pipeline for batch support. My initial implementation was with a copy of SilTranslator, but due to its lack of batch support, I could make the tests pass, but it failed when used on Serval.

My understanding of the changes in transformers 5 is just that they removed the Text2TextGenerationPipeline, with their suggestion being to use an LLM instead - see https://github.com/huggingface/transformers/blob/main/MIGRATION_GUIDE_V5.md#text-pipelines-that-should-just-be-llms. I don't think the LLM approach will work for us just yet?

My actual porting of TranslationPipeline wasn't too involved - most of the differences are me stripping out code that is unnecessary or unused, and combining TranslationPipeline and Text2TextGenerationPipeline.

---

.gitignore line 147 at r2 (raw file):

Previously, mshannon-sil wrote…

Do we not want a standard pyrightconfig.json for the repository?

I've removed this line - I can't find the file.

[ddaspit]

@ddaspit reviewed 18 files and all commit messages, made 4 comments, and resolved 13 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Enkidu93, mshannon-sil, and pmachapman).

[mshannon-sil]

machine/translation/huggingface/transformers_compatibility.py line 1 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I needed to implement Pipeline for batch support. My initial implementation was with a copy of SilTranslator, but due to its lack of batch support, I could make the tests pass, but it failed when used on Serval.

My understanding of the changes in transformers 5 is just that they removed the Text2TextGenerationPipeline, with their suggestion being to use an LLM instead - see https://github.com/huggingface/transformers/blob/main/MIGRATION_GUIDE_V5.md#text-pipelines-that-should-just-be-llms. I don't think the LLM approach will work for us just yet?

My actual porting of TranslationPipeline wasn't too involved - most of the differences are me stripping out code that is unnecessary or unused, and combining TranslationPipeline and Text2TextGenerationPipeline.

Is there a way that Serval is calling the pipeline that is unique to Serval and not how machine.py works? I've looked through the machine.py master branch, and it seems _TranslationPipeline was only ever called on a single batch at a time, since it was called inside of try_translate_n_batch. If Serval works differently, is there a reason why it's not going through translate_n_batch to take care of the batching? Does it need different batching logic?

[mshannon-sil]

machine/translation/huggingface/transformers_compatibility.py line 1 at r2 (raw file):

Previously, mshannon-sil wrote…

Is there a way that Serval is calling the pipeline that is unique to Serval and not how machine.py works? I've looked through the machine.py master branch, and it seems _TranslationPipeline was only ever called on a single batch at a time, since it was called inside of try_translate_n_batch. If Serval works differently, is there a reason why it's not going through translate_n_batch to take care of the batching? Does it need different batching logic?

And yes, your understanding of the changes are correct. We're also not at a spot to replace it with an LLM call.

[pmachapman]

@pmachapman made 5 comments, resolved 6 discussions, and dismissed @mshannon-sil from a discussion.
Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on ddaspit, Enkidu93, and mshannon-sil).

[ddaspit]

@ddaspit reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Enkidu93 and mshannon-sil).

---

machine/translation/huggingface/transformers_compatibility.py line 1 at r2 (raw file):

Previously, mshannon-sil wrote…

And yes, your understanding of the changes are correct. We're also not at a spot to replace it with an LLM call.

One of the key benefits of reimplementing TranslationPipeline is that it helps us to maintain backwards compatibility. Batching is an example of this. There are two levels of batching at play here. The first is at the Machine API level. This is used to support the streaming APIs in Machine. The second is at the Huggingface level. A model will often have a batch size for training/inferencing that needs to be configured to maximize speed and memory usage for a model. This is a separate batch size that can be configured for a particular translation engine. To keep Machine flexible, we want to support both types of batches. There are other configuration options that can be passed to the pipeline that we want to continue to support for compatibility reasons (see pipeline_kwargs argument in the HuggingFaceNmtEngine constructor).

[mshannon-sil]

@mshannon-sil made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Enkidu93 and pmachapman).

---

machine/translation/huggingface/transformers_compatibility.py line 1 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

One of the key benefits of reimplementing TranslationPipeline is that it helps us to maintain backwards compatibility. Batching is an example of this. There are two levels of batching at play here. The first is at the Machine API level. This is used to support the streaming APIs in Machine. The second is at the Huggingface level. A model will often have a batch size for training/inferencing that needs to be configured to maximize speed and memory usage for a model. This is a separate batch size that can be configured for a particular translation engine. To keep Machine flexible, we want to support both types of batches. There are other configuration options that can be passed to the pipeline that we want to continue to support for compatibility reasons (see pipeline_kwargs argument in the HuggingFaceNmtEngine constructor).

Okay that makes sense, thanks. Do we want to rename this file to something like hugging_face_nmt_pipeline.py to be more descriptive and match the neighboring files in machine/translation/huggingface?

[mshannon-sil]

machine/translation/huggingface/transformers_compatibility.py line 1 at r2 (raw file):

Previously, mshannon-sil wrote…

Okay that makes sense, thanks. Do we want to rename this file to something like hugging_face_nmt_pipeline.py to be more descriptive and match the neighboring files in machine/translation/huggingface?

The renaming I think is optional, so I'll go ahead and approve the PR.

[claude]

1. Verdict: request changes.
2. Most important: F1 (machine/translation/huggingface/hugging_face_nmt_engine.py:103). The engine crashes with AttributeError on the invalid-language path when given a model path, and two existing tests fail on this head.
3. Counts: Critical 2 (F1, F2), Important 4 (F3–F6), Low 3 (F7–F9).
4. Ran: poetry run pytest tests/translation/huggingface tests/jobs → 2 failed, 28 passed (test_construct_invalid_lang[True] and [False], both F1). Loading T5ForConditionalGeneration._from_config(cfg, attn_implementation='sdpa') on transformers 5.14.1 → ValueError (F2). generate on stas/tiny-m2m_100 with 2 beams → sequences [4,10], beam_indices [4,9] (F3).
5. Not verified: I did not run ./local_check.sh or --agent-strict. I did not load real t5-small/mT5 checkpoints (F2 was reproduced from a local config). F3's alignment effect is reasoned from tensor shapes. F7 and F8 are unmeasured.

API / wheel surface changed:

• HuggingFaceNmtModelTrainer.train() checkpoint and output_dir behavior (F5).
• add_lang_code_to_tokenizer (F7).
• HuggingFaceNmtEngine now changes a caller's model attention implementation (F4).
• The transformers major-version bump in pyproject.toml.
• _TranslationPipeline renamed to public SilTranslationPipeline.
• group_by_length build option replaced by train_sampling_strategy (F6).

I did not check parity with sillsdev/machine; the HF engine is Python-only.

Out of scope: the OOM-retry pipeline rebuild at hugging_face_nmt_engine.py:161 omits mpn=self._mpn. That predates this PR, which only renamed the class.

Findings: F1 unverified-fix pending (confirmed defect), F2 confirmed, F3 unverified, F4 accepted, F5 accepted, F6 accepted, F7 unverified, F8 unverified, F9 accepted. Each awaits the author's answer; I will mark them changed once addressed.

[claude]

Review summary

1. Verdict: request changes.
2. Most important: F1. Beam-search word alignments are shifted by one step (machine/translation/huggingface/hugging_face_nmt_engine.py:326), and the test's expected alignments were updated to match the shifted output.
3. Counts: 1 Critical (F1), 3 Important (F2, F3, F4), 3 Low (F5, F6, F7).
4. Ran:
   • pytest tests/translation/huggingface: 16 passed.
   • Inspected generate() output on transformers 5.14.1: beam_indices is [2, 9] against sequences [2, 10], which confirms F1.
   • Loaded hf-internal-testing/tiny-random-t5: attn_implementation='sdpa' raises ValueError, None loads, which confirms F2.
   • Called HuggingFaceNmtEngine without max_length, and with output_attentions=False: both work.
5. Not verified:
   • ./local_check.sh --agent-strict needed approval that wasn't granted in this session, so it did not run.
   • F3 is from reading the code, not reproduced.
   • No GPU was available, so the OOM paths are untested.

Public API / wheel surface changed:

• HuggingFaceNmtModelTrainer no longer resumes from checkpoints automatically (F4).
• HuggingFaceNmtEngine no longer loads non-SDPA architectures when output_attentions=False (F2).
• The transformers major-version bump changes a dependency of the published wheel.
• Parity with sillsdev/machine: none verified (the HF engine is Python-only).

Dropped after verification:

• A check_inputs crash on max_length=None: not reproduced, the pipeline fills in 20.
• output_attentions precedence: pipeline kwargs populate generation_config.
• Missing mpn in the OOM rebuild: already missing before this PR.

Status: F1 changed-requested, F2 answer-needed, F3 unverified, F4 answer-needed, F5 accepted, F6 accepted, F7 accepted (predates this PR).

🤖 Generated with Claude Code

[pmachapman]

@pmachapman resolved 16 discussions.
Reviewable status: 14 of 18 files reviewed, all discussions resolved (waiting on ddaspit, Enkidu93, and mshannon-sil).

[claude]

Review summary (round 6, follow-up)

1. Verdict: approve with fixes.
2. Most important: round-2 F2 and round-3 F2, machine/translation/huggingface/hugging_face_nmt_engine.py:320. The exported SilTranslationPipeline raises AssertionError when output_attentions is unset, or when it is built with True and called with False. Branching on attentions is not None fixes both cases.
3. Counts (open): Critical 0, Important 3 defects (round-2 F2 / round-3 F2; round-3 F5 = round-4 F4 = round-5 F3), Low 2 (round-2 F7, round-5 F7). No new findings this round: the previous round already reviewed 8ac226f, and nothing has been pushed since.
4. Ran:
   • poetry run pytest -q tests/translation/huggingface tests/jobs: 30 passed.
   • One-off repros on transformers 5.14.1 with stas/tiny-m2m_100 and hf-internal-testing/tiny-random-t5. The T5 prefix now works, close() restores sdpa, a failed constructor leaves eager, both output_attentions cases above assert, and T5 with output_attentions=False raises ValueError.
   • Read the transformers v4.47.1 and v4.57.1 sources: beam_indices already started at the first generated token in v4.
5. Not verified: ./local_check.sh --agent-strict needed approval that was not granted, so it did not run. No GPU was available, so the OOM paths were not exercised. Whether the re-baselined alignments are correct is still unverified.

Surface changed: the huggingface extra of the sil-machine wheel now pins transformers 5, and SilTranslationPipeline is newly exported. HuggingFaceNmtModelTrainer.train() no longer auto-resumes (accepted; belongs in the release notes). HuggingFaceNmtEngine no longer loads non-SDPA architectures (T5/mT5) with output_attentions=False (accepted as by design). The group_by_length option is replaced by train_sampling_strategy. Parity with sillsdev/machine: none verified, since this code is Python-only.

Withdrawn this round: the beam-alignment offset (round-3 F1, round-4 F3, round-5 F1). It predates this PR, because v4.47.1 beam_search.py:404 uses the same layout and the base code used the same slice. It is still a real bug and worth its own issue.

Noted, not posted (Low, on code the last round already saw): a call-time prefix sets self.prefix permanently (transformers_compatibility.py:34), so later calls without a prefix still prepend it.

Status. Earlier rounds each restarted at F1, so they are labelled here by round: R1 09-28 20:23, R2 09-29 20:37, R3 21:43, R4 22:05, R5 22:13.

• R1: F1 addressed, F2 accepted, F3 accepted, F4 accepted (unverified), F5 addressed, F6 accepted, F7 accepted.
• R2: F1 addressed, F2 open, F3 addressed, F4 accepted, F5 accepted, F6 accepted, F7 open, F8 accepted, F9 addressed.
• R3: F1 withdrawn, F2 open, F3 accepted, F4 accepted, F5 open, F6 accepted, F7 accepted, F8 accepted, F9 accepted, F10 withdrawn.
• R4: F1 addressed, F2 accepted (a Major that stays Blocking in Reviewable until a maintainer dismisses it), F3 withdrawn, F4 open, F5 accepted, F6 accepted, F7 accepted, F8 accepted (unverified), F9 accepted.
• R5: F1 withdrawn, F2 accepted, F3 open, F4 accepted, F5 accepted, F6 accepted, F7 open.

Reviewed at 8ac226f

🤖 Generated with Claude Code

[ddaspit]

@ddaspit reviewed 4 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Enkidu93 and mshannon-sil).

[mshannon-sil]

@mshannon-sil made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Enkidu93).

[ddaspit]

In the interest of trying to wrap up this PR, I added a commit that addresses some of the remaining Claude comments. I resolved the duplicates and created separate issues for the pre-existing issues that were identified in the review.

@ddaspit reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Enkidu93).

[claude]

Minor: F11. This guard misses the SDPA case: generate returns cross_attentions as a tuple of empty tuples, not None, so attentions[0][0] on the next line raises IndexError. Repro on stas/tiny-m2m_100: load with attn_implementation="sdpa", then call SilTranslationPipeline(..., output_attentions=True), and the call raises IndexError: tuple index out of range. Checking attentions and attentions[0] would fall back to empty alignments.

[claude]

FYI: F12. close() restores the saved implementation even when this engine did not change it, so two engines on one caller model break each other. Repro: engines A and B on an SDPA stas/tiny-m2m_100, both with output_attentions=True. B saves eager, A.close() sets sdpa, then B.translate(...) raises IndexError (via F11). Restoring only when this engine made the switch would avoid it.

[claude]

Review summary (round 7, follow-up on fcf9486)

1. Verdict: approve with fixes.
2. Most important: F11, machine/translation/huggingface/hugging_face_nmt_engine.py:328. The new attentions is not None guard lets through the tuple of empty tuples that SDPA returns, so the exported SilTranslationPipeline raises IndexError on an SDPA model with output_attentions=True.
3. Counts (new this round): Critical 0, Important 1 (F11), Low 1 (F12). No earlier finding is still open without an answer.
4. Ran:
   • poetry run pytest -q tests/translation/huggingface tests/jobs: 42 passed, including the new tests for T5 loading, the failed constructor, close() restore, pipeline output_attentions, and the group_by_length mapping.
   • One-off repros on transformers 5.14.1 with stas/tiny-m2m_100. With SDPA, generate(output_attentions=True).cross_attentions[0] is (), and the pipeline raises IndexError (F11). With two engines on one caller model, A.close() resets it to sdpa and B.translate raises IndexError (F12).
   • /code-review high: its other 8 findings duplicate existing threads or are on code unchanged since 8ac226f, so they were dropped.
5. Not verified: ./local_check.sh --agent-strict needed approval that was not granted, so it did not run (no black/flake8/isort/pyright result). No GPU, so the OOM paths were not exercised. Whether the re-baselined alignment strings are correct is still unchecked (R3 F6).

Surface changed: unchanged from round 6. The huggingface extra pins transformers 5. SilTranslationPipeline is newly exported (F11 is in it). HuggingFaceNmtModelTrainer.train() no longer auto-resumes (accepted). group_by_length is replaced by train_sampling_strategy, and an explicit non-default strategy now wins. Engines with output_attentions=False no longer force sdpa, so T5/mT5 load again. Parity with sillsdev/machine: none verified, since this code is Python-only.

Status. Earlier rounds restarted at F1, so they are labelled by round (R1 09-28 20:23; R2–R5 09-29 20:37, 21:43, 22:05, 22:13). This round continues from the highest number.

• R1: F1 addressed, F2 addressed (fcf9486 no longer forces sdpa), F3 accepted, F4 accepted (unverified), F5 addressed, F6 accepted, F7 accepted.
• R2: F1 addressed, F2 addressed (fcf9486, branches on returned attentions), F3 addressed, F4 accepted, F5 addressed (fcf9486), F6 accepted, F7 accepted (thread resolved without a code change), F8 addressed (fcf9486 test and precedence), F9 addressed.
• R3: F1 withdrawn, F2 addressed (fcf9486), F3 addressed (fcf9486, null no longer remaps), F4 accepted, F5 addressed (fcf9486, eager switch moved last), F6 accepted (now partly covered), F7 addressed (fcf9486), F8 accepted, F9 accepted, F10 withdrawn.
• R4: F1 addressed, F2 addressed (fcf9486, T5 test passes), F3 withdrawn, F4 addressed (fcf9486), F5 accepted, F6 addressed (fcf9486), F7 accepted, F8 accepted (unverified), F9 accepted.
• R5: F1 withdrawn, F2 addressed (fcf9486), F3 addressed (fcf9486), F4 accepted, F5 accepted, F6 addressed (fcf9486), F7 accepted.
• R7: F11 new, F12 new.

Reviewed at fcf9486

🤖 Generated with Claude Code