Skip to content

fix-qwen2vl-no-position_ids#33487

Merged
ArthurZucker merged 1 commit intohuggingface:mainfrom
simonJJJ:fix_qwen2vl_no_position_ids
Oct 29, 2024
Merged

fix-qwen2vl-no-position_ids#33487
ArthurZucker merged 1 commit intohuggingface:mainfrom
simonJJJ:fix_qwen2vl_no_position_ids

Conversation

@simonJJJ
Copy link
Contributor

@simonJJJ simonJJJ commented Sep 14, 2024

What does this PR do?

Fixes # (issue) (issue)

We encountered some issues with position_ids==None when fine-tuning the qwen2vl model, because the model_inputs do not pass the position_ids arg.

Before submitting

  • [] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

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.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Makes sense! Do you want to add a small test for training? 🤗

lancerts added a commit to linkedin/Liger-Kernel that referenced this pull request Oct 1, 2024
…one (#276)

## Summary
When `position_ids` is None, we should call `get_rope_index` to create
3D rope index

The code was copied from here:
huggingface/transformers#33487.

## Testing Done
I am using qwen2-vl to train the grounding task. The red box shows the
results before fixing, and the green box shows the results after fixing
(correct results).

<img width="146" alt="WechatIMG4500_副本"
src="https://github.com/user-attachments/assets/e7a42f89-e19b-4b53-b84a-0d62d981e54a">

- Hardware Type: 3090
- [x] run `make test` to ensure correctness
- [x] run `make checkstyle` to ensure code style
- [x] run `make test-convergence` to ensure convergence

---------

Co-authored-by: Shao Tang <tangshao28@gmail.com>
@zhangfaen
Copy link

zhangfaen commented Oct 13, 2024

I am trying to finetune Qwen2-VL for Object detection on coco dataset ( https://github.com/zhangfaen/finetune-Qwen2-VL )

Without this PR, my finetuned model performance is bad.
With this PR (by cherry picking), my finetuned model performance is really good.

Any progress for this PR to be merged?

Thank you

@zytx121
Copy link

zytx121 commented Oct 14, 2024

I am trying to finetune Qwen2-VL for Object detection on coco dataset ( https://github.com/zhangfaen/finetune-Qwen2-VL )

Without this PR, my finetuned model performance is bad. With this PR (by cherry picking), my finetuned model performance is really good.

Any progress for this PR to be merged?

Thank you

@zhangfaen Strange, after I modified this code, the fine-tuning performance still did not improve. Is there anything else that needs to be modified?

@zhangfaen
Copy link

I am trying to finetune Qwen2-VL for Object detection on coco dataset ( https://github.com/zhangfaen/finetune-Qwen2-VL )
Without this PR, my finetuned model performance is bad. With this PR (by cherry picking), my finetuned model performance is really good.
Any progress for this PR to be merged?
Thank you

@zhangfaen Strange, after I modified this code, the fine-tuning performance still did not improve. Is there anything else that needs to be modified?

for what task and against what dataset are you finetuning qwen2-vl ?

@simonJJJ
Copy link
Contributor Author

hi @ArthurZucker, would u mind taking a look at merging this PR? The community is waiting for it.

@zhangfaen
Copy link

hi @ArthurZucker, would u mind taking a look at merging this PR? The community is waiting for it.

+1 @ArthurZucker

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Yep sorry I approved I don't know why it was lost in tracks!

@ArthurZucker ArthurZucker merged commit 0ab0a42 into huggingface:main Oct 29, 2024
@Betty-J
Copy link

Betty-J commented Nov 14, 2024

Yep sorry I approved I don't know why it was lost in tracks!

Hi there, when I used pip install transformers, I installed version 4.46.2, which still does not include the updates mentioned here https://github.com/huggingface/transformers/blob/main/src/transformers/models/qwen2_vl/modeling_qwen2_vl.py#L1723.

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