Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generate does not take into account config.decoder.eos_token_id #14905

Closed
NielsRogge opened this issue Dec 23, 2021 · 7 comments
Closed

Generate does not take into account config.decoder.eos_token_id #14905

NielsRogge opened this issue Dec 23, 2021 · 7 comments
Labels
Good First Issue

Comments

@NielsRogge
Copy link
Contributor

As reported by some people (see NielsRogge/Transformers-Tutorials#53 and on the forum), the generate() method currently does not take into account config.decoder.eos_token_id, only config.eos_token_id to properly stop generation.

Hence, models that are made using EncoderDecoderModel/VisionEncoderDecoderModel/SpeechEncoderDecoderModel will not properly stop generation if config.eos_token_id is not set.

cc @patrickvonplaten @patil-suraj
@patrickvonplaten
Copy link
Member

Hmm, yeah I think I'm fine with adding some if - statements to the generate() method

@patrickvonplaten
Copy link
Member

Do you want to open a PR for it? :-)

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@thinksoso
Copy link
Contributor

i will try to fix it

@thinksoso thinksoso mentioned this issue Jan 29, 2022
Merged
@thinksoso
Copy link
Contributor

thinksoso commented Jan 29, 2022 •
edited

I pulled a request #15403, but the ci failed. By analysing the ci failture log, I find that it has a hidden logic, if you don't pass a eos_pos_id, you want the model to generate until max-length. That is what the code do

transformers/tests/test_modeling_encoder_decoder.py

Line 404 in 16d4acb

self.assertEqual(generated_output.shape, (input_ids.shape[0],) + (decoder_config.max_length,))

So, adding self.config.decoder.eos_pos_id simply is not enough.
cc @NielsRogge @patrickvonplaten

@NielsRogge NielsRogge mentioned this issue Mar 4, 2022
Closed
@Batese2001
Copy link

Hi, the above pull request mentioned offhandedly that this had been fixed, is that the case or is this still open as indicated?

@NielsRogge
Copy link
Contributor Author

Seems like this got fixed, closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Good First Issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@patrickvonplaten @thinksoso @NielsRogge @Batese2001