-
Notifications
You must be signed in to change notification settings - Fork 31.7k
fix gemma3n case failure #41426
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
fix gemma3n case failure #41426
Conversation
Signed-off-by: Yao, Matrix <[email protected]>
Signed-off-by: Yao, Matrix <[email protected]>
|
seems the ci failures are not related to my PR. |
src/transformers/generation/utils.py
Outdated
| if ( | ||
| custom_gen_config_value == global_default_value | ||
| (not has_custom_gen_config_value) | ||
| and custom_gen_config_value == global_default_value |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gante
left a comment
There was a problem hiding this 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 :)
|
Actually, we can see in the CI that these changes break other cases :'( @yao-matrix , let's instead add |
|
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 <[email protected]>
done, pls review again, and do you need me create an issue to track the |
Signed-off-by: Yao, Matrix <[email protected]>
|
@zucchini-nlp @gante , could you pls help review again? Thx |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: gemma3n |
* fix gemma3n case failure Signed-off-by: Yao, Matrix <[email protected]> * fix style Signed-off-by: Yao, Matrix <[email protected]> * 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 <[email protected]> * fix style Signed-off-by: Yao, Matrix <[email protected]> --------- Signed-off-by: Yao, Matrix <[email protected]> Co-authored-by: Marc Sun <[email protected]>
* fix gemma3n case failure Signed-off-by: Yao, Matrix <[email protected]> * fix style Signed-off-by: Yao, Matrix <[email protected]> * 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 <[email protected]> * fix style Signed-off-by: Yao, Matrix <[email protected]> --------- Signed-off-by: Yao, Matrix <[email protected]> Co-authored-by: Marc Sun <[email protected]>
Problem
pytest -rA tests/models/gemma3n/test_modeling_gemma3n.py::Gemma3nIntegrationTest::test_generation_beyond_sliding_window_with_generation_configgenerate different output evendo_sample=Falseingeneration_config = GenerationConfig(max_new_tokens=20, do_sample=False), which makes UT failsRoot 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.