Migrate to transformers 5 by pmachapman · Pull Request #363 · sillsdev/machine.py

pmachapman · 2026-09-10T02:43:27Z

This change is

codecov-commenter · 2026-09-10T05:19:14Z

Codecov Report

❌ Patch coverage is 79.68750% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.10%. Comparing base (e2af2c8) to head (29f5034).

Files with missing lines	Patch %	Lines
...tion/huggingface/hugging_face_nmt_model_trainer.py	63.63%	16 Missing ⚠️
...nslation/huggingface/transformers_compatibility.py	77.77%	14 Missing ⚠️
...translation/huggingface/hugging_face_nmt_engine.py	88.67%	6 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.10%   -0.03%     
==========================================
  Files         390      391       +1     
  Lines       24644    24704      +60     
==========================================
+ Hits        22705    22753      +48     
- Misses       1939     1951      +12

☔ 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 · 2026-09-10T19:41:37Z

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.

ddaspit · 2026-09-14T21:38:02Z

-            return BatchEncoding(batch_outputs, tensor_type=return_tensors)
+            return tokenizer(
+                batch_tokens,
+                is_split_into_words=True,


is_split_into_words=True re-runs the pre-tokenizer on each piece, so continuation pieces get a ▁ prepended and get re-split. I checked this against nllb-200-distilled-600M: ['▁tele', 'phones'] comes back as ['▁tele', '▁ph', 'ones'], and the gloss pieces ['tele', 'phone'] come back as ['▁tele', '▁phone'], which undoes the ▁ stripping above. Any batch with a key-term row trains on the wrong ids.

Can we go back to convert_tokens_to_ids and build the input_ids/attention_mask dict by hand? That still works in v5.

Done. prepare_for_model is removed, but _encode_plus is approximately the same.

ddaspit · 2026-09-14T21:38:02Z

        if self.model.config.decoder_start_token_id is not None:
            start_index = 1
-        if generate_kwargs["output_attentions"] is True:
+        if self.generation_config.output_attentions:


The old code defaulted output_attentions to True here. self.generation_config.output_attentions defaults to None in v5, so anyone who doesn't pass the flag now gets empty alignments. Meanwhile line 53 still picks eager attention when the flag is missing, so we pay for eager and get nothing from it. Should the two agree on a default?

I've made the default True.

ddaspit · 2026-09-14T21:38:02Z

+            model_config = AutoConfig.from_pretrained(str(model), label2id={}, id2label={}, num_labels=0)
+
+            # If output_attentions is True or None, we need to set the attn_implementation to eager to get the attentions
+            attn_implementation = "eager" if self._pipeline_kwargs.get("output_attentions", True) else "sdpa"


This only sets eager when loading from a path. If someone passes a model object (which HuggingFaceNmtModel does) with output_attentions=True, the model stays on sdpa, sdpa returns None for the weights, and attentions[0][0] blows up. v4 fell back to eager here; v5 doesn't. Maybe call set_attn_implementation("eager") on that branch too.

Maybe call set_attn_implementation("eager") on that branch too.

Done.

ddaspit · 2026-09-14T21:38:02Z

+    # Make sure the docstring is updated when the default generation config is changed (in all pipelines in this file)
+    _default_generation_config = GenerationConfig(
+        max_new_tokens=256,
+        num_beams=4,


num_beams=4 here overrides the model's own config because the pipeline merge only fills in None values. So an engine built without num_beams now does 4-beam search. That's why test_translate_greedy needed new expected output and a real sequence confidence: it isn't greedy anymore. I'd drop num_beams from this default and put the greedy test back.

I removed num_beams and the only value which reverted in the test was the translated string, while the confidence value went really weird - 1.39. Running the tests on a GPU were fine, but on a CPU, Math Overflows occurred.

These defaults came straight from the code in transformers v4 for Text2TextGenerationPipeline, so I imagine they were used? I have kept them unless you notice something else is causing the odd behavior.

Removing num_beams caused it to use a greedy search, which had a bug that is fixed in my commit.

ddaspit · 2026-09-14T21:38:02Z

  huggingface:
    parent_model_name: facebook/nllb-200-distilled-1.3B
    train_params:
+      auto_find_batch_size: true


The trainer we removed doubled gradient_accumulation_steps every time it halved the batch, so the effective batch stayed at 64. The HF version just shrinks the batch (by 0.9x per retry in accelerate 1.14, so it can take six or more retries to get from 64 to 32) and leaves accumulation alone. With max_steps: 5000 that means fewer examples seen and a warmup/cosine schedule tuned for a batch we're no longer using. Is that intended? If so, a comment here would help.

Also, TrainParams doesn't expose auto_find_batch_size, so clients can't turn it off.

After some experimentation, I have reverted to the previous AutoGradientAccumulationStepsSeq2SeqTrainer, and updated it to work correctly with the new version of transformers.

ddaspit · 2026-09-14T21:38:02Z

-        generate_kwargs["output_attentions"] = generate_kwargs.get("output_attentions", True)
-        self.check_inputs(input_length, generate_kwargs["min_length"], generate_kwargs["max_length"])
-        output = self.model.generate(
+            input_tokens = [self.tokenizer.convert_ids_to_tokens(seq) for seq in model_inputs["input_ids"]]


preprocess always sets input_tokens, so this fallback can't run. If it ever did, it'd include special tokens and disagree with the filtering in postprocess. Suggest just model_inputs.pop("input_tokens") like before.

ddaspit · 2026-09-14T21:38:02Z

+        self._pipeline = SilTranslationPipeline(
            model=self._model,
-            tokenizer=self._tokenizer,
+            tokenizer=cast(PreTrainedTokenizer, self.tokenizer),


This casts to PreTrainedTokenizer but line 151 passes the raw tokenizer. In v5 the fast tokenizer isn't a subclass of PreTrainedTokenizer, so the cast is lying. Suggest typing the pipeline param as PreTrainedTokenizerBase and passing self._tokenizer in both places.

ddaspit · 2026-09-14T21:38:02Z

        self._model = model
        if isinstance(model, PreTrainedModel):
-            self._model_path = Path(model.name_or_path)
+            self._model_path = Path(str(model.name_or_path))


Path(str(None)) turns a missing name_or_path into a directory called "None". Better to raise here. Also, dropping StrPath means this no longer accepts str under pyright while everything else in the package does.

I've added a None check, and added str to the allowable model values. StrPath caused pyright to throw an error.

ddaspit · 2026-09-14T21:38:02Z

      gradient_accumulation_steps: 1
      label_smoothing_factor: 0.2
-      group_by_length: true
+      train_sampling_strategy : group_by_length


Nit: stray space before the colon.

ddaspit · 2026-09-14T21:38:02Z

            vocab = tokenizer.get_vocab().keys()
            charset = set()
-            mpn_normalize = True if isinstance(tokenizer, (NllbTokenizerFast)) else False
+            mpn_normalize = True if isinstance(tokenizer, NllbTokenizer) else False


Nit: True if isinstance(...) else False is just isinstance(...).

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.

pmachapman · 2026-09-17T03:38:05Z

    gradient_accumulation_steps: int | None = None
    label_smoothing_factor: float | None = None
-    group_by_length: bool | None = None
+    train_sampling_strategy: str | None = None


Done. I've added it and updated HuggingFaceNmtModelFactory to migrate this property to train_sampling_strategy and warn the user it is deprecated.

pmachapman · 2026-09-17T03:38:05Z

  huggingface:
    parent_model_name: facebook/nllb-200-distilled-1.3B
    train_params:
+      auto_find_batch_size: true


After some experimentation, I have reverted to the previous AutoGradientAccumulationStepsSeq2SeqTrainer, and updated it to work correctly with the new version of transformers.

pmachapman · 2026-09-17T03:38:06Z

      gradient_accumulation_steps: 1
      label_smoothing_factor: 0.2
-      group_by_length: true
+      train_sampling_strategy : group_by_length


pmachapman · 2026-09-17T03:38:06Z

+            model_config = AutoConfig.from_pretrained(str(model), label2id={}, id2label={}, num_labels=0)
+
+            # If output_attentions is True or None, we need to set the attn_implementation to eager to get the attentions
+            attn_implementation = "eager" if self._pipeline_kwargs.get("output_attentions", True) else "sdpa"


Maybe call set_attn_implementation("eager") on that branch too.

Done.

pmachapman · 2026-09-17T03:38:06Z

+        self._pipeline = SilTranslationPipeline(
            model=self._model,
-            tokenizer=self._tokenizer,
+            tokenizer=cast(PreTrainedTokenizer, self.tokenizer),


pmachapman · 2026-09-17T03:38:08Z

            vocab = tokenizer.get_vocab().keys()
            charset = set()
-            mpn_normalize = True if isinstance(tokenizer, (NllbTokenizerFast)) else False
+            mpn_normalize = True if isinstance(tokenizer, NllbTokenizer) else False


pmachapman · 2026-09-17T03:38:08Z

-            return BatchEncoding(batch_outputs, tensor_type=return_tensors)
+            return tokenizer(
+                batch_tokens,
+                is_split_into_words=True,


Done. prepare_for_model is removed, but _encode_plus is approximately the same.

pmachapman · 2026-09-17T03:38:08Z

+    # Make sure the docstring is updated when the default generation config is changed (in all pipelines in this file)
+    _default_generation_config = GenerationConfig(
+        max_new_tokens=256,
+        num_beams=4,


I removed num_beams and the only value which reverted in the test was the translated string, while the confidence value went really weird - 1.39. Running the tests on a GPU were fine, but on a CPU, Math Overflows occurred.

These defaults came straight from the code in transformers v4 for Text2TextGenerationPipeline, so I imagine they were used? I have kept them unless you notice something else is causing the odd behavior.

pmachapman · 2026-09-17T03:38:09Z

+        super().__init__(**kwargs)
+        self.framework = framework
+
+    def _sanitize_parameters(


pmachapman · 2026-09-17T03:38:09Z


        assert finetuned_result_nochar != finetuned_result_char
-        assert finetuned_result_nochar_composite != finetuned_result_char_composite
+        assert finetuned_result_nochar_composite == finetuned_result_char_composite


I don't have a clear idea - the closest to a reason I can come to is the difference in normalizer. This test stumped me for a while.

claude · 2026-09-17T22:12:03Z

+    def preprocess(
+        self,
+        *args,
+        truncation=TruncationStrategy.DO_NOT_TRUNCATE,
+        src_lang: str | None = None,
+        tgt_lang: str | None = None,
+    ):
+        if self.tokenizer is None:
+            raise RuntimeError("No tokenizer is specified.")
+        build_inputs = getattr(self.tokenizer, "_build_translation_inputs", None)
+        if callable(build_inputs):
+            build_inputs_fn = cast(Callable[..., Any], build_inputs)
+            return build_inputs_fn(
+                *args, return_tensors="pt", truncation=truncation, src_lang=src_lang, tgt_lang=tgt_lang
+            )


This port of transformers v4's TranslationPipeline.preprocess drops the else fallback the original had:

else: return super()._parse_and_tokenize(*args, truncation=truncation)

_build_translation_inputs is only implemented by multilingual tokenizers (NLLB, M2M100, mBART...). For any other tokenizer — including T5/mT5, which hugging_face_nmt_engine.py explicitly supports via the translate {src} to {tgt}: prefix — preprocess now implicitly returns None. SilTranslationPipeline.preprocess (in hugging_face_nmt_engine.py) calls super().preprocess(...) and then accesses inputs.encodings, which will raise AttributeError: 'NoneType' object has no attribute 'encodings' for these models. The existing tests only exercise NLLB/M2M100 tokenizers, so this path isn't covered.

transformers_compatibility.py#L104-L118

claude · 2026-09-17T22:12:10Z

+        # Allow group_by_length backwards compatibility
+        if "group_by_length" in args:
+            warnings.warn(
+                "'group_by_length' is deprecated and will be removed in a future release.",
+                category=DeprecationWarning,
+                stacklevel=2,
+            )
+            if args.pop("group_by_length") is True:
+                args["train_sampling_strategy"] = "group_by_length"
        # Use "max_steps" from root for backward compatibility


This shim only handles group_by_length: true; when a caller explicitly passes group_by_length: false, the key is popped but nothing maps it to a non-grouping train_sampling_strategy. settings.yaml now defaults train_sampling_strategy: group_by_length, and since Dynaconf merges user overrides on top of that default, input {"group_by_length": false} leaves train_sampling_strategy as "group_by_length" — grouping stays enabled, the opposite of what group_by_length: false used to do (disable the old default-on grouping).

Needs an else branch mapping False to a non-grouping strategy value.

hugging_face_nmt_model_factory.py#L30-L39

claude · 2026-09-17T22:12:20Z

            model=model,
            args=self._training_args,
            train_dataset=cast(Any, train_dataset),
-            tokenizer=tokenizer,
            data_collator=data_collator,
            callbacks=[
                _ProgressCallback(


tokenizer=tokenizer was removed from this Seq2SeqTrainer construction but never replaced — transformers v5 renamed this parameter to processing_class. AutoGradientAccumulationStepsSeq2SeqTrainer.__init__ (below, around line 464) also no longer accepts or forwards a tokenizer/processing_class, so Trainer.processing_class stays None. Trainer.save_model()/trainer.save() only persist tokenizer files via self.processing_class.save_pretrained(...), so the output directory ends up with no tokenizer files (unless the separate add_tokens() path happens to run). Loading the saved model afterwards (e.g. HuggingFaceNmtEngine(output_dir) → AutoTokenizer.from_pretrained(...)) will then fail with OSError: Can't load tokenizer.

Fix: pass processing_class=tokenizer here, and add a processing_class parameter to AutoGradientAccumulationStepsSeq2SeqTrainer.__init__ that forwards it to super().__init__().

hugging_face_nmt_model_trainer.py#L379-L384

- fix test_update_tokenizer_missing_char

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

ddaspit · 2026-09-18T03:18:06Z

-                normalize_logits=True,
-            ),
-        )
+        try:


I realized that this exception was added in silnlp to deal with an OOM error that can occur when retrieving the scores. It would fallback to computing the transition scores on the CPU, when an OOM error occurred on the GPU. I added a commit that has what I think is a better fix for this problem. It provides our own implementation of the function that retrieves just the scores we want.

ddaspit · 2026-09-18T03:18:07Z

+    # Make sure the docstring is updated when the default generation config is changed (in all pipelines in this file)
+    _default_generation_config = GenerationConfig(
+        max_new_tokens=256,
+        num_beams=4,


Removing num_beams caused it to use a greedy search, which had a bug that is fixed in my commit.

ddaspit · 2026-09-18T03:18:07Z


        assert finetuned_result_nochar != finetuned_result_char
-        assert finetuned_result_nochar_composite != finetuned_result_char_composite
+        assert finetuned_result_nochar_composite == finetuned_result_char_composite


This was caused by a regression in MbartTokenizer in transformers 5. I implemented a workaround.

claude · 2026-09-18T03:31:30Z

+    def preprocess(
+        self,
+        *args,
+        truncation=TruncationStrategy.DO_NOT_TRUNCATE,
+        src_lang: str | None = None,
+        tgt_lang: str | None = None,
+    ):
+        if self.tokenizer is None:
+            raise RuntimeError("No tokenizer is specified.")
+        build_inputs = getattr(self.tokenizer, "_build_translation_inputs", None)
+        if callable(build_inputs):
+            build_inputs_fn = cast(Callable[..., Any], build_inputs)
+            return build_inputs_fn(
+                *args, return_tensors="pt", truncation=truncation, src_lang=src_lang, tgt_lang=tgt_lang
+            )


Bug: preprocess silently returns None for tokenizers without _build_translation_inputs

This ported preprocess only returns a value when self.tokenizer defines _build_translation_inputs (true for multilingual tokenizers like M2M100/NLLB/mBART). For T5, mT5, and Marian-style tokenizers — which don't define that method — the function falls off the end and implicitly returns None.

The caller, SilTranslationPipeline.preprocess in hugging_face_nmt_engine.py#L237-L245, immediately accesses inputs.encodings, which raises AttributeError: 'NoneType' object has no attribute 'encodings'.

This is a reachable path: HuggingFaceNmtEngine explicitly builds a translation prefix for t5-/google/mt5- models at hugging_face_nmt_engine.py#L80-L87, so this crashes for those models. Upstream transformers v4's TranslationPipeline.preprocess had an else: return super()._parse_and_tokenize(*args, truncation=truncation) fallback that wasn't carried over in this port (and can't be restored as-is, since this class extends Pipeline rather than Text2TextGenerationPipeline). Existing tests only exercise stas/tiny-m2m_100, which does define _build_translation_inputs, so this gap isn't caught by CI.

claude · 2026-09-18T03:32:33Z

        args: Seq2SeqTrainingArguments,
        data_collator: Any,
        train_dataset: Optional[Dataset] = None,
-        eval_dataset: Optional[Union[Dataset, Dict[str, Dataset]]] = None,
-        tokenizer: Optional[PreTrainedTokenizerBase] = None,
-        model_init: Optional[Callable[[], PreTrainedModel]] = None,
-        compute_metrics: Optional[Callable[[EvalPrediction], Dict]] = None,
        callbacks: Optional[List[TrainerCallback]] = None,
-        optimizers: Tuple[Optional[Optimizer], Optional[LambdaLR]] = (None, None),
-        preprocess_logits_for_metrics: Optional[Callable[[Tensor, Tensor], Tensor]] = None,
    ):
        super().__init__(
-            model,
-            args,
-            data_collator,
-            train_dataset,  # type: ignore
-            eval_dataset,  # type: ignore
-            tokenizer,
-            model_init,
-            compute_metrics,
-            callbacks,
-            optimizers,  # type: ignore
-            preprocess_logits_for_metrics,
+            model=model,
+            args=args,
+            data_collator=data_collator,
+            train_dataset=train_dataset,
+            callbacks=callbacks,
        )


Bug: tokenizer is dropped instead of renamed to processing_class, so fine-tuned models save without tokenizer files

This diff removes the tokenizer parameter/kwarg here and at the construction call site (L378-L390) instead of renaming it to transformers 5's processing_class. In transformers 5, Trainer._save() only writes tokenizer files (tokenizer.json, tokenizer_config.json, etc.) when self.processing_class is set.

HuggingFaceNmtModelTrainer.save() only calls self._trainer.save_model() / save_metrics() / save_state(), so with no processing_class set, the output directory ends up with model weights but no tokenizer files. The only other tokenizer.save_pretrained call in this file (line 171) is inside the add_tokens helper, which only runs when add_unk_src_tokens/add_unk_tgt_tokens is set and missing characters are found, so it doesn't cover the default path.

This breaks reloading: HuggingFaceNmtEngine.__init__ calls AutoTokenizer.from_pretrained(name_or_path) on that same output directory, which will fail once a fine-tuned model is loaded from it. Fix: pass processing_class=tokenizer through both this subclass's __init__ and the constructor call, in place of the removed tokenizer parameter.

mshannon-sil · 2026-09-19T00:53:36Z

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 · 2026-09-19T00:54:59Z

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 marked this pull request as ready for review September 10, 2026 03:31

pmachapman requested review from Enkidu93 and ddaspit September 10, 2026 03:31

pmachapman force-pushed the transformers_5 branch from cf019f7 to 2d7748c Compare September 10, 2026 05:17

Enkidu93 requested a review from mshannon-sil September 10, 2026 19:41

Enkidu93 approved these changes Sep 14, 2026

View reviewed changes

ddaspit reviewed Sep 14, 2026

View reviewed changes

mshannon-sil requested changes Sep 15, 2026

View reviewed changes

pmachapman added 4 commits September 17, 2026 07:23

Migrate to transformers 5

9b74583

Use forward-ported pipeline

7f6ccd9

Use auto_find_batch_size

ec98af8

Revert Use auto_find_batch_size

3b7d9ec

pmachapman force-pushed the transformers_5 branch from 1c4d641 to 870183e Compare September 17, 2026 03:06

Fixes from code review

355ffd8

pmachapman force-pushed the transformers_5 branch from 870183e to 355ffd8 Compare September 17, 2026 03:28

pmachapman requested review from Enkidu93, ddaspit and mshannon-sil September 17, 2026 03:38

pmachapman commented Sep 17, 2026

View reviewed changes

More memory efficient method for retrieving token scores

6b95bfa

claude Bot reviewed Sep 17, 2026

View reviewed changes

Remove default generation config

9515787

- fix test_update_tokenizer_missing_char

ddaspit approved these changes Sep 18, 2026

View reviewed changes

claude Bot reviewed Sep 18, 2026

View reviewed changes

Fix type checking issues on Mac

29f5034

Uh oh!

Conversation

pmachapman commented Sep 10, 2026 • edited by ddaspit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Enkidu93 commented Sep 10, 2026

Uh oh!

Enkidu93 left a comment

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshannon-sil left a comment

Choose a reason for hiding this comment

Uh oh!

pmachapman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pmachapman commented Sep 10, 2026 •

edited by ddaspit

Loading

codecov-commenter commented Sep 10, 2026 •

edited

Loading