Skip to content
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

Pr 3002 ci branch #3008

Closed
wants to merge 4 commits into from
Closed

Pr 3002 ci branch #3008

wants to merge 4 commits into from

Conversation

drbh
Copy link
Collaborator

@drbh drbh commented Feb 10, 2025

This PR re opens #3002 to run CI and add some small changes to align the logic across different hardware

@sywangyi
Copy link
Contributor

Hi, @drbh the change of 7c09eae for rocm and ipex is incorrect, because it's inplace ops ( ops.rotary_embedding/ipex.llm.functional.rotary_embedding) for query/key. but you change query/key tensor in
query = torch.cat([-query[..., rot:], query[..., :rot]], dim=-1)
key = torch.cat([-key[..., rot:], key[..., :rot]], dim=-1)

For Qwen vl rope, I think it's same with llama except sin/cos calulation. that's why I delete forward and torch cat for sin/cos.

@drbh
Copy link
Collaborator Author

drbh commented Feb 11, 2025

Hi @sywangyi you are completely correct! apologies for my misunderstanding the forward and cat should be removed - I've reverted the changed here and if CI passes I'll merge the original PR. Thanks for this fix 🙏

@drbh
Copy link
Collaborator Author

drbh commented Feb 12, 2025

closing as the original PR was merged #3002

@drbh drbh closed this Feb 12, 2025
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.

2 participants