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

Add repetition_penalty aligned with huggingface #866

Closed

Conversation

Abraham-Xu
Copy link
Contributor

@Abraham-Xu Abraham-Xu commented Aug 25, 2023

In huggingface/transformers generate method:
https://github.com/huggingface/transformers/blob/ae320fa53f74cc4dfa0e4fc3c95b6129a86b0512/src/transformers/generation/utils.py#L1295

  1. it prepares a logits_processor from generation_config
    https://github.com/huggingface/transformers/blob/ae320fa53f74cc4dfa0e4fc3c95b6129a86b0512/src/transformers/generation/utils.py#L1540
  2. it does logits pre-process by logits_processor in greedy_search/sample/beam_search
    https://github.com/huggingface/transformers/blob/ae320fa53f74cc4dfa0e4fc3c95b6129a86b0512/src/transformers/generation/utils.py#L2457

Specificaly, repetition penalty is a frequently used pre-process method in logits_processor. It prevents repetitions through a penalty. This penalized sampling works by discounting the scores of previously generated tokens. (https://arxiv.org/pdf/1909.05858.pdf)

I introduced the the repetition penalty pre-process to sampler.py aligned with huggingface implementation:
https://github.com/huggingface/transformers/blob/ae320fa53f74cc4dfa0e4fc3c95b6129a86b0512/src/transformers/generation/logits_process.py#L328

@Abraham-Xu Abraham-Xu force-pushed the add_repetition_penalty branch 4 times, most recently from 8e78387 to b48f6b0 Compare August 27, 2023 12:55
Copy link

@Scagin Scagin left a comment

Choose a reason for hiding this comment

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

I am waiting for this, I need some method to make hf and vllm compat when I use penalty arguments

@Abraham-Xu
Copy link
Contributor Author

I have tested the repetition_penalty under greedy search mode by Llama 7b and Baichuan models. The results are all aligned with huggingface transformers.

Co-authored-by: Dong-Yong Lee <[email protected]>
@markluofd
Copy link

this pr solved my problem, for my llama2 model, repetition_penalty is necessary, otherwise the result is incorrect

@WoosukKwon WoosukKwon self-requested a review September 1, 2023 06:20
@TheBloke
Copy link

TheBloke commented Sep 2, 2023

Great! Looking forward to testing this as my perception is that vLLM is very poor with repetition currently, and that the existing repetition penalty params (freqency_penalty etc) do little or nothing.

Although part of that problem is that there's no per-request seed, something we also really need.

@jeffchy
Copy link

jeffchy commented Sep 4, 2023

Beautiful!

@BaiMoHan
Copy link

BaiMoHan commented Sep 4, 2023

Very nice

@BaiMoHan
Copy link

BaiMoHan commented Sep 12, 2023

Hey, what's wrong?I did not find the repetition_penalty attribute in sampling_params.py when I use v0.1.7. I'm a bit impatient.🥺🥺🥺

@@ -162,30 +170,61 @@ def _apply_penalties(
indices.append(i)

# Return early if all sequences have zero penalties.

Choose a reason for hiding this comment

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

this comment is misleading now?

logits[indices] -= frequency_penalties.unsqueeze(dim=1) * bin_counts
presence_mask = (bin_counts > 0.0).to(dtype=logits.dtype)
logits[indices] -= presence_penalties.unsqueeze(dim=1) * presence_mask
else:
Copy link

@leshanbog leshanbog Sep 13, 2023

Choose a reason for hiding this comment

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

why else? will presence_penalty and repetition_penalty work together?

@heshuguo
Copy link

@WoosukKwon please check if the fix can be merged.

@WoosukKwon
Copy link
Collaborator

@Abraham-Xu @heshuguo This PR is a bit in conflict with #1048, which tries to refactor the sampler code. @zhuohan123 Can we merge this PR before yours?

@WoosukKwon
Copy link
Collaborator

Hi @Abraham-Xu, sorry for the late response.

Could you update the PR? There are some merge conflicts because we refactored sampler in #1048.

@jessiewiswjc
Copy link

I want to know if presence_penalty/frequency_penalty in vllm can work with repetition_penalty in hf at the same time?

@zhuohan123
Copy link
Member

Supported with #1424. But again, thank you for your contribution! Let us know if there is any other issue.

@zhuohan123 zhuohan123 closed this Oct 29, 2023
@Abraham-Xu
Copy link
Contributor Author

Supported with #1424. But again, thank you for your contribution! Let us know if there is any other issue.

Sorry for late reply, busy with working and other things recently. It is great to support repetition penalty in any way.

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.