fixed max_length in beam_search() and group_beam_search() to use beam…

[GeetDsa]

…_scorer.max_length

What does this PR do?

Fixes the issue #11040
beam_search() and group_beam_search() uses beam_scorer.max_length if max_length is not explicitly passed.

Fixes #11040

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• [] Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors which may be interested in your PR.

[patil-suraj]

hi @GeetDsa

Thanks a lot for the PR. I understand the issue and IMO what should be done here is to make sure to pass the same max_length to the BeamScorer and beam_search instead of changing the method.

This is because the overall philosophy of generate is that whenever some argument is None its value should explicitly default to the value specified in config. This how all generation methods work.

[patrickvonplaten]

Thanks for the issue & PR @GeetDsa! I agree with @patil-suraj that we should not change the way max_length is set in beam_search.

Overall, the problem IMO is actually that BeamScorer has a max_length attribute... => this shouldn't be the case IMO:

• BeamHypotheses has a max_length attribute that is unused and can be removed
• BeamSearchScorer has a max_length attribute that is only used for the function finalize => the better approach here would be too pass max_length as an argument to finalize(...) IMO

This solution will then ensure that only one max_length is being used and should also help to refactor out max_length cc @Narsil longterm.

Do you want to give it a try @GeetDsa ? :-)

[GeetDsa]

Thanks for the issue & PR @GeetDsa! I agree with @patil-suraj that we should not change the way max_length is set in beam_search.

Overall, the problem IMO is actually that BeamScorer has a max_length attribute... => this shouldn't be the case IMO:

• BeamHypotheses has a max_length attribute that is unused and can be removed
• BeamSearchScorer has a max_length attribute that is only used for the function finalize => the better approach here would be too pass max_length as an argument to finalize(...) IMO

This solution will then ensure that only one max_length is being used and should also help to refactor out max_length cc @Narsil longterm.

Do you want to give it a try @GeetDsa ? :-)

I can give a try :)

[Narsil]

BeamHypotheses has a max_length attribute that is unused and can be removed

Nice !

BeamSearchScorer has a max_length attribute that is only used for the function finalize => the better approach here would be too pass max_length as an argument to finalize(...) IMO

Seems easier.
@GeetDsa Do you think you could also add a test that reproduces your issue without your fix and that passes with the fix ? That will make backward compatibility easier to test (we're heading towards a direction to remove max_length as much as possible while maintaining backward compatbility)

[GeetDsa]

I have created a new pull request #11378 ; @Narsil, I think it will be little hard and time consuming for me to implement a test as I am not well-versed with it.

[github-actions]

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.