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

Generate: unify LogitsWarper and LogitsProcessor #32626

Merged
merged 6 commits into from
Aug 16, 2024

Conversation

gante
Copy link
Member

@gante gante commented Aug 12, 2024

What does this PR do?

LogitsProcessor and LogitsWarper were originally created as two separate classes, with LogitsWarper being a LogitsProcessor that should only be applied to sample-based decoding methods. In practice, they are exactly the same.

Having two separate classes meant that there was undesired code duplication: two functions to instantiate these objects, two separate pieces of logic to apply these processors, etc.

⚠️ Perhaps more importantly, given that the order of the logit processors is important, having two separate types of objects results in needing custom logic if we want to interleave the two types of instances. Since we don't want custom logic, this PR is a requirement for an ongoing project integration.

This PR:

  • removes all internal uses of LogitsWarper and related logic, moving it to the logic related to LogitsProcessor
  • keeps LogitsWarper public with a deprecation warning, for BC

@gante gante requested a review from LysandreJik August 12, 2024 13:31
@HuggingFaceDocBuilderDev

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.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Indeed, that's much cleaner! Thanks for the cleanup @gante

@gante gante merged commit 70d5df6 into huggingface:main Aug 16, 2024
24 checks passed
@gante gante deleted the unify_logits branch August 16, 2024 10:20
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.

3 participants