Repository navigation
fix(pipeline): preserve model.generation_config precedence over pipeline defaults (#47752) - #47953
Conversation
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
oops, forgot to submit comments
| if user_generation_config is not None: | ||
| prepared_generation_config, kwargs = self.model._prepare_generation_config( | ||
| generation_config=user_generation_config, **kwargs | ||
| ) | ||
| else: | ||
| base_config = copy.deepcopy(self.model.generation_config) |
There was a problem hiding this comment.
we defi need a comment here showing expected prio order:
user_config > pipeline_default > model_config > generation_config_defaults
There was a problem hiding this comment.
Added the priority order comment as requested!
| if user_generation_config is not None: | ||
| prepared_generation_config, kwargs = self.model._prepare_generation_config( | ||
| generation_config=user_generation_config, **kwargs | ||
| ) | ||
| else: | ||
| base_config = copy.deepcopy(self.model.generation_config) | ||
| if default_pipeline_generation_config is not None: | ||
| base_config.update( | ||
| **default_pipeline_generation_config.to_dict(), | ||
| defaults_only=True, | ||
| allow_custom_entries=True, | ||
| ) | ||
| prepared_generation_config, kwargs = self.model._prepare_generation_config( | ||
| generation_config=base_config, **kwargs | ||
| ) |
There was a problem hiding this comment.
i think all we need is to merge "user + pipe_default" to make a single config and pass down the line, no?
There was a problem hiding this comment.
Updated to the unified single-path merge logic as suggested!
| def test_pipeline_respects_model_generation_config(self): | ||
| """Test for #47752: Verify model.generation_config modifications take precedence over pipeline defaults.""" | ||
| model_id = "hf-internal-testing/tiny-random-gpt2" | ||
| model = AutoModelForCausalLM.from_pretrained(model_id) |
There was a problem hiding this comment.
and for test, lets also check different orders of inputs to make sure the priority-list takes effect
There was a problem hiding this comment.
Expanded the unit test to verify all priority order combinations!
…xpand priority order unit tests
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
Niiiice, i think this is the best we can do while keeping BC, though I have to admit the whole generation-config legacy is confusing
@Rocketknight1 for another review
|
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. |
Rocketknight1
left a comment
Rocketknight1
left a comment
There was a problem hiding this comment.
Agree that it's confusing but this PR is definitely an improvement on discarding model.generation_config, so I'm happy to accept!
Co-authored-by: Matt <Rocketknight1@users.noreply.github.com>
CI recapDashboard: View test results in Grafana |
Fixes #47752
What does this PR do?
This PR fixes incorrect precedence of
generation_configvalues inPipeline.__init__(src/transformers/pipelines/base.py).Currently, when a user modifies
model.generation_config(e.g. settingmodel.generation_config.max_new_tokens = 500) before creating a pipeline, the pipeline'sgeneration_configfails to reflect these values becausedefault_pipeline_generation_config(which sets defaults likemax_new_tokens = 256) was applied as the base config inside_prepare_generation_config()withdefaults_only=True. Sincedefault_pipeline_generation_configalready set non-Nonevalues,defaults_only=Truesilently ignored the user'smodel.generation_configsettings.Fix
Instead of starting with the pipeline's default config and applying
model.generation_configwithdefaults_only=True, we now:copy.deepcopy(self.model.generation_config), preserving any explicit user settings on the model.None) fields usingdefault_pipeline_generation_configwithdefaults_only=True.kwargspassed topipeline(...)at the highest priority.This aligns execution with the documented priority order:
**kwargsVerification
Verified locally against text generation and speech (
Whisper) pipelines:# All checks & tests pass python -m ruff check src/transformers/pipelines/base.py tests/pipelines/test_pipelines_text_generation.py git diff --checkAdded regression unit test
test_pipeline_respects_model_generation_configtotests/pipelines/test_pipelines_text_generation.py.Who can review?
cc @Rocketknight1 @zucchini-nlp