Multiple fixes to FA tests in AMD#40498
Conversation
|
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. |
remi-or
force-pushed
the
sweep-amd-ci-fa
branch
from
31df0bd to
3b2d07a
Compare
| # Default vision config is None to avoid a mutable default argument | ||
| if vision_config is None: | ||
| vision_config = { | ||
| "depth": 2, |
There was a problem hiding this comment.
should not be a problem as long as the QwenConfig takes the dict and creates a new VisionConfig from it, but agreed that mutable defaults is not a good idea in general
There was a problem hiding this comment.
Yeah it's kind of a nit, I thought it was the problem initially and it was not, but since it's fixed might as well leave it!
There was a problem hiding this comment.
Let's avoid any mutable type in the arguments 🙏 so keep as it is in this PR
|
Although you are a AMD fans, don't forget to use |
remi-or
force-pushed
the
sweep-amd-ci-fa
branch
2 times, most recently
from
ee12748 to
5f91da4
Compare
|
|
||
| EXPECTED_DECODED_TEXT = [ | ||
| 'system\nYou are a helpful assistant.\nuser\nWhat kind of dog is this?\nassistant\nThe dog in the picture appears to be a Labrador Retriever. Labradors are known for their friendly and energetic nature, which is evident in', | ||
| "system\nYou are a helpful assistant.\nuser\nWho are you?\nassistant\n�\n\n addCriterion\nI'm sorry, but I don't understand your question. Could you please provide more context or clarify what you're asking", |
There was a problem hiding this comment.
@zucchini-nlp the second answer seems wrong to me, it used to be I am Qwen, a large language model created by Alibaba Cloud. I am designed to answer a wide range of questions and provide information on various topics which makes more sense. I added a #FIXME because it seems out of scope for this PR
There was a problem hiding this comment.
yeah, this is quite similar to the generation from non-FA2 model a few lines above, though I have no idea when it got from I am Qwen to addCriterion
Thanks, will need to go back in git history to see what changed
|
This comment contains run-slow, running the specified jobs: models: ['models/gemma3', 'models/qwen2_5_omni', 'models/qwen2_5_vl'] |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
approving in advance, so it can be merged as long as the slow tests show ✅
remi-or
force-pushed
the
sweep-amd-ci-fa
branch
from
58b11bc to
e27a65d
Compare
|
run-slow: gemma3, qwen2_5_omni, qwen2_5_vl |
|
This comment contains run-slow, running the specified jobs: models: ['models/gemma3', 'models/qwen2_5_omni', 'models/qwen2_5_vl'] |
remi-or
force-pushed
the
sweep-amd-ci-fa
branch
from
e27a65d to
27801c5
Compare
|
run-slow: gemma3, qwen2_5_omni, qwen2_5_vl |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: gemma3, qwen2_5_omni, qwen2_5_vl |
|
This comment contains run-slow, running the specified jobs: models: ['models/gemma3', 'models/qwen2_5_omni', 'models/qwen2_5_vl'] |
|
There are slow tests failing for
|
4 participants
This PR fixes a tests across a few models in the following ways:
qwen2_5_omnigemma3,qwen2_5_omniandqwen2_5_vlqwen2_5_omniandqwen2_5_vlFA tests: the test changes thehidden_sizeso it breaks compatibility withmrope. For this issue, I added a fix usingtryandexceptto ensure the change tomrope_sectiondoes not propagate to the rest of the tests. I am not sure this is the best way, so tagging @ydshiehThis PR touches mostly test-related files, so I think it's ok to bundle everything, but please let me know if not. As the non test-related stuff this is mostly multimodal, cc. @zucchini-nlp