Skip to content

Fix ProphetNet forward to handle tuple encoder_outputs#39794

Open
Abdennacer-Badaoui wants to merge 13 commits intohuggingface:mainfrom
Abdennacer-Badaoui:fix/prophetnet-encoder-outputs-tuple
Open

Fix ProphetNet forward to handle tuple encoder_outputs#39794
Abdennacer-Badaoui wants to merge 13 commits intohuggingface:mainfrom
Abdennacer-Badaoui:fix/prophetnet-encoder-outputs-tuple

Conversation

@Abdennacer-Badaoui
Copy link
Copy Markdown
Member

This PR fixes a bug in ProphetNet where passing encoder_outputs as a tuple to forward() would raise:

AttributeError: 'tuple' object has no attribute 'last_hidden_state'

This happens because the forward() method assumes encoder_outputs is a BaseModelOutput when provided manually, while the docstring says it can be a tuple.

Changes:

Added a check in forward():

Convert tuple encoder_outputs to BaseModelOutput format before passing to ProphetNetSeq2SeqModelOutput.

if isinstance(encoder_outputs, tuple):
    encoder_outputs = BaseModelOutput(
        last_hidden_state=encoder_outputs[0],
        hidden_states=encoder_outputs[1] if len(encoder_outputs) > 1 else None,
        attentions=encoder_outputs[2] if len(encoder_outputs) > 2 else None,
    )

@Abdennacer-Badaoui Abdennacer-Badaoui marked this pull request as draft July 30, 2025 15:55
@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Abdennacer-Badaoui Abdennacer-Badaoui force-pushed the fix/prophetnet-encoder-outputs-tuple branch 2 times, most recently from 9299165 to feb83dd Compare July 30, 2025 17:17
@Abdennacer-Badaoui Abdennacer-Badaoui marked this pull request as ready for review July 30, 2025 17:45
@github-actions github-actions Bot requested a review from ArthurZucker July 30, 2025 17:45
@Rocketknight1
Copy link
Copy Markdown
Member

cc @ArthurZucker for text models

Copy link
Copy Markdown
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd rather update the docstring as it should no longer be a tuple!

@Abdennacer-Badaoui
Copy link
Copy Markdown
Member Author

Alright! In this case, the ProphetNetEncoder should be modified to always return a BaseModelOutput. This means removing the return_dict argument, as it will not be used there.

@@ -1056,7 +1056,6 @@ def forward(
inputs_embeds: Optional[torch.Tensor] = None,
output_attentions: Optional[bool] = None,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw if you have time, can you add the check_model_input decorator, updating to the latest standards? 🤗

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, what I mean by that is add this, remove the args, add the TransformersKwargs, remove the

        output_attentions = output_attentions if output_attentions is not None else self.config.output_attentions
        output_hidden_states = (
            output_hidden_states if output_hidden_states is not None else self.config.output_hidden_states
        )

etc! Have a look at modeling llama to see how we handle ti

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, it’s clearer now. I’ll try to fix it. Thanks!

@github-actions
Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: prophetnet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants