Fix remaining issues in beam score calculation#27808
Fix remaining issues in beam score calculation#27808ArthurZucker merged 6 commits intohuggingface:mainfrom
Conversation
gante
left a comment
There was a problem hiding this comment.
Good catch, thanks for fixing!
BTW, run RUN_SLOW=1 py.test tests/models/vision_encoder_decoder/test_modeling_vision_encoder_decoder.py::ViT2GPT2ModelIntegrationTest::test_inference_coco_en -- this test may need an update in its value
There was a problem hiding this comment.
If I'm not mistaken, this is the same as cur_len (L228). I'd suggest renaming cur_len into generated_len, which is more representative of the variable contents!
There was a problem hiding this comment.
Yes, the input_ids[batch_beam_idx].shape[-1] should be same for each batch_beam_idx, so we can simply use cur_len here.
There was a problem hiding this comment.
I'd add a note that the else case here exists for retrocompatibility reasons :)
There was a problem hiding this comment.
Thanks for the reminder. Added!
@gante I have updated the expectation value for this test. I have also incorporated all your suggestions. Ready to go! |
gante
left a comment
There was a problem hiding this comment.
Perfect, thanks for iterating 💛
Note for @ArthurZucker: there may be slow CI failures due to this change. Although I suspect there won't, since the correction is small. In any case, I'll keep an eye on the CI after this gets merged.
There was a problem hiding this comment.
nit: we can actually remove this if/else, if the result is back into being the same :P
There was a problem hiding this comment.
redundant if/else removed
…he decoder prompt
|
All suggested changes are incorporated. Ready to go! @gante @ArthurZucker |
ArthurZucker
left a comment
There was a problem hiding this comment.
thanks for this! 🤗
| score = sum_logprobs / ((hyp.shape[-1] - decoder_prompt_len) ** self.length_penalty) | ||
| if generated_len is not None: | ||
| score = sum_logprobs / (generated_len**self.length_penalty) | ||
| # This 'else' case exists for retrocompatibility |
There was a problem hiding this comment.
| # This 'else' case exists for retrocompatibility | |
| # This 'else' case exists for backward compatibility |
There was a problem hiding this comment.
oops, PR already merged, maybe let's stay with it for now?
| if is_pt: | ||
| expectation = 20 | ||
| else: | ||
| # TODO (joao): fix me | ||
| expectation = 13 |
What does this PR do?
This PR further fixes the remaining issues in beam score calculation following #27351 .
More specifically:
hypinprocessdoes not include the next generated token on which the current beam score is calculated, but thehypinfinalizeincludes all the generated tokens so far. This inconsistency is resolved by changing theaddfunction ofBeamHypotheses. Now we directly pass the current length of the generated tokens toadd.is_donefunction ofBeamHypotheses, we are directly usingmax_lengthwithout deductingdecoder_prompt_len. It is fixed now.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@gante