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] Logits Processor Zoo with HF Transformers #2524

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ariG23498
Copy link
Contributor

Adding a blog post on logits processor zoo from NVIDIA and its usage with Hugging Face transformers.

@ariG23498 ariG23498 marked this pull request as ready for review December 10, 2024 09:25
Copy link
Member

@Vaibhavs10 Vaibhavs10 left a comment

Choose a reason for hiding this comment

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

Great job on putting this together, it already reads well. I added some suggestion/ nits.

logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Show resolved Hide resolved
logits-processor-zoo.md Show resolved Hide resolved
@ariG23498 ariG23498 requested a review from Vaibhavs10 December 10, 2024 11:57
Copy link
Member

@Vaibhavs10 Vaibhavs10 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for incorporating the suggestions:

Two final suggestions:

  1. Put a more generic title - if possible.
  2. After each generation, add a line that tells people what to look for specifically in that generation and how it relates to the LogitProcessor.

logits-processor-zoo.md Outdated Show resolved Hide resolved
@ariG23498
Copy link
Contributor Author

@Vaibhavs10 thank you for the review! I have updated the blog according to your review.

Copy link
Member

@Vaibhavs10 Vaibhavs10 left a comment

Choose a reason for hiding this comment

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

Thank you! <3

@ariG23498
Copy link
Contributor Author

Waiting for the NVIDIA team to review. After that is done we can merge this.

Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

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

I think some additional contextualization could be helpful, as expressed in a comment.

_blog.yml Outdated
title: "Controlling Language Model Generation with NVIDIA's LogitsProcessorZoo"
author: ariG23498
thumbnail: /blog/assets/logits-processor-zoo/thumbnail.png
date: December 11, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Reminder to update

Comment on lines 6 to 8
- user: aerdem
guest: true
org: nvidia
Copy link
Member

Choose a reason for hiding this comment

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

This author profile does not seem to belong to the nvidia org.

logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
ariG23498 and others added 2 commits December 17, 2024 16:11
Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
Copy link
Member

@gante gante left a comment

Choose a reason for hiding this comment

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

+1 to Pedro's contextualization comment

If a reader skims the top, it gives the impression that transformers doesn't include advanced logits processors (which is obviously not true 😛 ). Instead, I would try to frame Nvidia's library as an expansion to the existing processors, and would also highlight that generate is compatible with custom logits processors like Nvidia's

logits-processor-zoo.md Outdated Show resolved Hide resolved
logits-processor-zoo.md Outdated Show resolved Hide resolved
@huggingface huggingface deleted a comment from code30x58 Dec 17, 2024
@ariG23498
Copy link
Contributor Author

@pcuenca @gante thank you for the thorough reviews. I have made the updates, could you all review the changes and let me know if the contextualization is better in this version of the blog post?

@ariG23498 ariG23498 requested review from pcuenca and gante December 18, 2024 06:33
Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

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

Checked the intro and skimmed through the rest, given that several of us had already reviewed. The context and framing look great to me now!

org: nvidia
---

# Controlling Language Model Generation with NVIDIA's LogitsProcessorZoo
Copy link
Member

Choose a reason for hiding this comment

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

I love how the intro provides great context now, thank you! 🙌

logits-processor-zoo.md Outdated Show resolved Hide resolved
Co-authored-by: Pedro Cuenca <pedro@huggingface.co>
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.

4 participants