Skip to content

Customizable Logit Warping Strategies for Generation #40010#40403

Open
PamelaBha wants to merge 20 commits intohuggingface:mainfrom
PamelaBha:Issue40010
Open

Customizable Logit Warping Strategies for Generation #40010#40403
PamelaBha wants to merge 20 commits intohuggingface:mainfrom
PamelaBha:Issue40010

Conversation

@PamelaBha
Copy link
Copy Markdown

What does this PR do?

Feature request
Improve the generate() API by supporting custom, declarative logit warping strategies. Make it easier for users to plug in standard and custom LogitProcessors via configuration or arguments without needing to subclass or dive into internals.

Motivation
The generation module already supports rich logit manipulation through LogitProcessorList, but:

It is undocumented and hard to use for casual users
Requires advanced subclassing to customize behaviors (e.g., word bans, domain constraints)
Doesn’t support JSON- or dict-style configuration like many other parts of Transformers
Making logit warping more accessible enables:

Prompt engineers and power users to fine-tune generation behavior
Safer generation via blacklists or probability shifting
Dynamic controls like repetition penalties or temperature annealing

Fixes # (issue): 40010

Before submitting

Who can review?

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

@Rocketknight1
Copy link
Copy Markdown
Member

cc @gante

Copy link
Copy Markdown
Contributor

@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.

Very cool PR! I like where this feature is going 🔥

I've added a few comments, but I think they are simple to solve 🤗 Let me know if you have questions/counter-proposals regarding my comments.

Other global comments:

  • Missing: documentation of logits_processor in GenerationConfig. I would add a link to the example file inside the docs, your examples are quite clear!
  • Suggestion: in the documentation of logits_processor in generate(), I would add a mention about the new format (and a link to the examples)
  • Some classes should be directly imported from the top level, i.e. we should be able to do from transformers import LogitProcessorRegistry. This is done by adding the classes to the outermost __init__.py. I think the only class we need to add there is LogitProcessorRegistry (we can already to from transformers import LogitsProcessor)

Comment thread .gitignore Outdated

if is_torch_available():
from .logits_process import SynthIDTextWatermarkLogitsProcessor, WatermarkLogitsProcessor
from ..cache_utils import (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(this diff seems related to a merge conflict, we can probably remove these added lines)

Comment on lines +568 to +570



Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +1164 to +1168
# Special handling for logit_processors - serialize to JSON string
if "logit_processors" in output and output["logit_processors"] is not None:
if not isinstance(output["logit_processors"], str):
# Convert to JSON string
output["logit_processors"] = json.dumps(output["logit_processors"])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this needed? if self.logit_processors is a list of dicts, then returning it directly should be fine :)

config._original_object_hash = hash(config) # Hash to detect whether the instance was modified
return config

def get_logit_processors(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we move this function to utils.py?

Goal: let's keep classes as independent as possible -- the generation config doesn't need to know about the expected format that logit_processors takes in generate. It's mostly a data storage class.


# Enhanced LogitsProcessorList
class ConfigurableLogitsProcessorList(LogitsProcessorList):
"""Extended LogitsProcessorList that supports configuration-based construction."""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing: an example in the docstring

Comment thread src/transformers/generation/utils.py Outdated

# Add any directly passed processors last
if logits_processor is not None:
processors.extend(logits_processor)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

merging with the custom logits_processor already done in self._merge_criteria_processor_list call a few lines above

Comment thread src/transformers/generation/utils.py Outdated
Comment on lines +1313 to +1325
if configured_processors is not None:
# Check for duplicates and warn
existing_types = {type(p).__name__ for p in processors}
config_types = {type(p).__name__ for p in configured_processors}
duplicates = existing_types & config_types

if duplicates:
warnings.warn(
f"Duplicate LogitProcessors detected: {duplicates}. "
f"Configured processors will be added after standard ones."
)

processors.extend(configured_processors)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps we can reuse the logic in _merge_criteria_processor_list ?

Comment thread tests/generation/test_logits_process.py Outdated
self.assertTrue(delay_pattern_processor.active_batches.all())
self.assertTrue((delay_pattern_processor.delay_pattern == torch.tensor(delay_pattern) - 1).all())

class TestLogitProcessorRegistry(unittest.TestCase):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's have a single class for everything related to the configurable logits processors 🤗

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Resolved

Comment on lines +69 to +70
from transformers.generation.logits_process import LogitsProcessor, LogitProcessorRegistry
import torch
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

all the imports in the example should be:
1 - at the top of the file
2 - transformers imports should be top-level imports, i.e. from transformers import LogitsProcessor, LogitProcessorRegistry. See the global PR comment I added about this 🤗

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Resolved

@PamelaBha
Copy link
Copy Markdown
Author

Very cool PR! I like where this feature is going 🔥

I've added a few comments, but I think they are simple to solve 🤗 Let me know if you have questions/counter-proposals regarding my comments.

Other global comments:

  • Missing: documentation of logits_processor in GenerationConfig. I would add a link to the example file inside the docs, your examples are quite clear!
  • Suggestion: in the documentation of logits_processor in generate(), I would add a mention about the new format (and a link to the examples)
  • Some classes should be directly imported from the top level, i.e. we should be able to do from transformers import LogitProcessorRegistry. This is done by adding the classes to the outermost __init__.py. I think the only class we need to add there is LogitProcessorRegistry (we can already to from transformers import LogitsProcessor)

thanks a lot for reviewing. I will address these soon and send a new PR

@PamelaBha
Copy link
Copy Markdown
Author

@gante this test is failing in the latest build but nothing to do with my change I believe: FAILED tests/models/gpt_oss/test_modeling_gpt_oss.py::GptOssModelTest::test_assisted_decoding_matches_greedy_search_1_same - AssertionError: False is not true

any pointers for me?

@PamelaBha
Copy link
Copy Markdown
Author

@gante this test is failing in the latest build but nothing to do with my change I believe: FAILED tests/models/gpt_oss/test_modeling_gpt_oss.py::GptOssModelTest::test_assisted_decoding_matches_greedy_search_1_same - AssertionError: False is not true

any pointers for me?

Ping on this @gante

@gante
Copy link
Copy Markdown
Contributor

gante commented Sep 9, 2025

@PamelaBha don't worry about tests that you believe are unrelated to your changes. That may be temporary issues in our codebase, and I can help you with them when we're ready to merge 🤗

A request: the diff in the PR has many style-related changes, likely the outcome of an automated tool (like the one I paste below). Make sure your environment is up-to-date (pip install -e .[dev]), and make sure the diff only contains strictly needed changes. Minimal diff = we can better track why changes were made = easier long-term maintenance :D

Screenshot 2025-09-09 at 16 27 28

Ping me when the PR is ready for a re-review

@PamelaBha
Copy link
Copy Markdown
Author

@gante can you help reopen my PR? I was trying to undo all the styling bot changes and somehow accidentally closed the PR. I am hoping you have a reopen permission

@gante
Copy link
Copy Markdown
Contributor

gante commented Sep 12, 2025

haha done 👍

@PamelaBha
Copy link
Copy Markdown
Author

@gante please take a look at the latest PR

@PamelaBha
Copy link
Copy Markdown
Author

@gante ping on this

@PamelaBha
Copy link
Copy Markdown
Author

Hi @gante - can you please help review and complete the PR?

@zucchini-nlp
Copy link
Copy Markdown
Member

@PamelaBha I will take a look tomorrow, sorry for a delay. With the major v5 release, we might have missed this PR. In the meanwhile, review would be easier if you can revert style changes which aren't related. I see the configuration file has no changes expect for style and shorter line length

It'll help me spot the actual files to review

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