Migrate to transformers 5

[pmachapman]

This change is 

[codecov-commenter]

Codecov Report

❌ Patch coverage is 85.89744% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.07%. Comparing base (96ec59f) to head (6d4cd7f).

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	95.00%	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.07%   -0.07%     
==========================================
  Files         390      394       +4     
  Lines       24644    24896     +252     
==========================================
+ Hits        22705    22922     +217     
- Misses       1939     1974      +35     

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

[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).

[ddaspit]

I changed my mind on how to handle eager attention and output_attentions. Instead of changing the passed in model, we throw if the attention type isn't compatible with outputting attentions.

@ddaspit reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: 18 of 19 files reviewed, 2 unresolved discussions (waiting on Enkidu93 and pmachapman).

[ddaspit]

@ddaspit reviewed 1 file.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Enkidu93 and pmachapman).

[claude]

Round 9 (915ca5a, 6d4cd7f since 5ec388b)

1. Verdict: approve with fixes.
2. Most important: F14, machine/translation/huggingface/hugging_face_nmt_engine.py:51. With the defaults, a caller-owned model loaded the usual way (transformers 5 picks sdpa) now raises ValueError in HuggingFaceNmtEngine and HuggingFaceNmtModel. It failed loudly, not silently, but callers need to know.
3. Counts: Critical 0, Important 1 (F14, new), Low 0. F11, F12 and R3 F6 were addressed and their threads resolved.
4. Ran: poetry run pytest -q tests/translation/huggingface tests/jobs gave 49 passed. black --check, flake8, isort --check-only and pyright on the two changed files were clean (pyright: 0 errors). A script loading stas/tiny-m2m_100 with no attn_implementation showed sdpa, and both the engine and HuggingFaceNmtModel.translate raised the new ValueError.
5. Not verified: I was not allowed to run ./local_check.sh --agent-strict, so there is no full-suite or strict result. There is no GPU, so the OOM paths were not exercised. Beam alignment strings still carry the offset that predates this PR (withdrawn beam threads).

Surface changed: HuggingFaceNmtEngine and SilTranslationPipeline no longer modify a caller-owned model. When output_attentions is on (the engine default), they now require eager attention and raise ValueError otherwise (F14). Everything else is as in round 8: transformers==5.14.1, datasets>=5.0.1,<6, SilTranslationPipeline newly exported, no auto-resume in HuggingFaceNmtModelTrainer.train() (accepted), and group_by_length replaced by train_sampling_strategy. The jobs path passes output_attentions=False, so it is unaffected. Parity with sillsdev/machine: none verified, since this code is Python-only.

Status. Rounds R1–R5 restarted at F1, so they are labelled by round.

• R1: F1 addressed, F2 addressed, F3 accepted, F4 accepted (unverified), F5 addressed, F6 accepted, F7 accepted.
• R2: F1 addressed, F2 addressed, F3 addressed, F4 accepted, F5 addressed, F6 accepted, F7 accepted, F8 addressed, F9 addressed.
• R3: F1 withdrawn, F2 addressed, F3 addressed, F4 accepted, F5 addressed, F6 addressed (915ca5a), F7 addressed, F8 accepted, F9 accepted, F10 withdrawn.
• R4: F1 addressed, F2 addressed, F3 withdrawn, F4 addressed, F5 accepted, F6 addressed, F7 accepted, F8 accepted (unverified), F9 accepted.
• R5: F1 withdrawn, F2 addressed, F3 addressed, F4 accepted, F5 accepted, F6 addressed, F7 accepted.
• R7: F11 addressed (915ca5a), F12 addressed (915ca5a).
• R8: F13 addressed.
• R9: F14 new.

Reviewed at 6d4cd7f

🤖 Generated with Claude Code