-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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 <matrix.yao@intel.com>
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.
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 <matrix.yao@intel.com>
done, pls review again, and do you need me create an issue to track the |
@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 <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>
Problem
pytest -rA tests/models/gemma3n/test_modeling_gemma3n.py::Gemma3nIntegrationTest::test_generation_beyond_sliding_window_with_generation_config
generate different output evendo_sample=False
ingeneration_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.