Skip to content

Conversation

yao-matrix
Copy link
Contributor

@yao-matrix yao-matrix commented Oct 7, 2025

Problem

pytest -rA tests/models/gemma3n/test_modeling_gemma3n.py::Gemma3nIntegrationTest::test_generation_beyond_sliding_window_with_generation_config generate different output even do_sample=False in generation_config = GenerationConfig(max_new_tokens=20, do_sample=False), which makes UT fails

Root cause

custom generation_config will be overode by model generation_config even it explicitly set, when the set value equals global default.

Fix

Add a condition to make the overriding only happens when the custom_generation_config doesn't explicitly set the field.

@SunMarc , pls help review, thx very much.

Signed-off-by: Yao, Matrix <matrix.yao@intel.com>
Signed-off-by: Yao, Matrix <matrix.yao@intel.com>
@yao-matrix
Copy link
Contributor Author

seems the ci failures are not related to my PR.

if (
custom_gen_config_value == global_default_value
(not has_custom_gen_config_value)
and custom_gen_config_value == global_default_value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the case:
When user sets generation_config = GenerationConfig(max_new_tokens=20, do_sample=False), here do_sample is False, while global_default_value is also False, and model_gen_config_value is True, the current condition cannot distinguish whether the value is explicitly set by user or just the global default, so it just modify it to model default. Add another condition to make sure it happens only when user didn't explicitly specify the value.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed this is an issue, and afair we discussed it with @gante after merging the PR with config value overwriting. cc @gante for review

@zucchini-nlp zucchini-nlp requested a review from gante October 8, 2025 09:06
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.

There are probably a few more cases where this inheritance is still broken, but the fix looks sensible in general :)

@gante
Copy link
Member

gante commented Oct 8, 2025

Actually, we can see in the CI that these changes break other cases :'(

@yao-matrix , let's instead add use_model_defaults=False in the model.generate call that is creating issues. We need to fully rewrite this attribute inheritance :(

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

generation_config way need re-visit

Signed-off-by: Yao, Matrix <matrix.yao@intel.com>
@yao-matrix
Copy link
Contributor Author

pytest -rA tests/models/gemma3n/test_modeling_gemma3n.py::Gemma3nIntegrationTest::test_generation_beyond_sliding_window_with_generation_config

done, pls review again, and do you need me create an issue to track the generation_config issue? It seems a HF recommended way to use generate in some docs and blogs.

@yao-matrix
Copy link
Contributor Author

@zucchini-nlp @gante , could you pls help review again? Thx

Copy link
Contributor

github-actions bot commented Oct 9, 2025

[For maintainers] Suggested jobs to run (before merge)

run-slow: gemma3n

@SunMarc SunMarc enabled auto-merge (squash) October 10, 2025 09:07
@SunMarc SunMarc merged commit f4487ec into huggingface:main Oct 10, 2025
19 checks passed
@yao-matrix yao-matrix deleted the issue-545 branch October 10, 2025 16:25
AhnJoonSung pushed a commit to AhnJoonSung/transformers that referenced this pull request Oct 12, 2025
* fix gemma3n case failure

Signed-off-by: Yao, Matrix <matrix.yao@intel.com>

* fix style

Signed-off-by: Yao, Matrix <matrix.yao@intel.com>

* Update dependency_versions_table.py

* change the case argument passing way to make the case PASS,
generation_config way need re-visit

Signed-off-by: Yao, Matrix <matrix.yao@intel.com>

* fix style

Signed-off-by: Yao, Matrix <matrix.yao@intel.com>

---------

Signed-off-by: Yao, Matrix <matrix.yao@intel.com>
Co-authored-by: Marc Sun <57196510+SunMarc@users.noreply.github.com>
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.

5 participants