Fix number of patch check for different vision feature select strategy #32494
Conversation
There was a problem hiding this comment.
Cool, thanks for working on this!
Yes, we'd need tests and you can add them in
Basically we'll run a tiny model with dummy inputs, and verify that changing select_strategy run without errors. I don't think we need slow tests here, just make sure after adding the test to run
RUN_SLOW=1 pytest tests/models/llava_next/test_modeling_llava_next.py
and check that all tests pass. If something doesn't pass, just let me know. Might be numerical precision error from different hardware as the PR doesn't change the model logic
|
@insujang hey, is the PR in progress or do you need help with adding tests? I can do the test part if you are busy :) |
|
Hi @zucchini-nlp , I am sorry but I am too busy to work on it :( I would sincerely appreciate it if you could do. Thank you! |
|
@insujang no worries, thanks a lot for contribution. Will add a test soon and merge it to |
There was a problem hiding this comment.
Perfect, LGTM! ❤️
I added a test as @insujang is busy and now this can be reviewed
| @@ -648,7 +648,7 @@ def _merge_input_ids_with_image_features( | |||
|
|
|||
| return final_embedding, final_attention_mask, position_ids, final_labels, final_input_ids | |||
|
|
|||
| def pack_image_features(self, image_features, image_sizes, image_newline=None): | |||
| def pack_image_features(self, image_features, image_sizes, vision_feature_select_strategy, image_newline=None): | |||
There was a problem hiding this comment.
Should it have "default" as a default kwarg value so as to not change the signature and keep usages of pack_image_features outside of the model working?
(Not necessary if this method should be considered internal)
There was a problem hiding this comment.
yes, should be internal as it doesn't make much sense outside of LLaVA
huggingface#32494) * Fix number of patch check for different vision feature select strategy * add test --------- Co-authored-by: raushan <raushan@huggingface.co>
What does this PR do?
Fixes #32395 (issue)
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
I think a new test for this change needs to be added (using
parameterizedto test differentvision_feature_select_strategyandvision_feature_layer), however, I have no idea how the new test would look like. With this change there will be no errors during forward, but do we also need to check the outputs with expected ones? Which test class would be suitable to have this test? @zucchini-nlp Appreciate it if you could share your thoughts regarding this. Thanks!Who can review?
@zucchini-nlp
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.