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

Fix: Calling update reshape attribute function for non reshape ops #1038

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

ashokkumarkannan1
Copy link
Contributor

@ashokkumarkannan1 ashokkumarkannan1 commented Jan 13, 2025

Fix: #816

This PR fixes the update_reshape_attr called for a non-reshape operation. This is from commute_utils in erase inverse ops logic, where this update_reshape_attr function called for squeeze, unsqueeze and transpose ops. So added a condition to only call this for reshape ops.

Logs

Tested for Bart model and verified.

Log Before Fix - test_bart_without_fix.log
Log After Fix - test_bart_with_fixt.log

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests758 ran487 passed271 skipped0 failed
TestResult
No test annotations available

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests689 ran424 passed265 skipped0 failed
TestResult
No test annotations available

@ashokkumarkannan1 ashokkumarkannan1 changed the title Condition based reshape attr update Fix: Calling update reshape attribute function for non reshape ops Jan 21, 2025
@ashokkumarkannan1 ashokkumarkannan1 force-pushed the akannan/fix_update_reshape_attr branch from dbd7efc to b02266e Compare January 21, 2025 07:24
@ashokkumarkannan1 ashokkumarkannan1 marked this pull request as ready for review January 21, 2025 07:25
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests665 ran434 passed231 skipped0 failed
TestResult
No test annotations available

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests823 ran490 passed333 skipped0 failed
TestResult
No test annotations available

@dgolubovicTT
Copy link
Contributor

@ashokkumarkannan1 how does this affect this PR #833?
Here, one of the problems was that we called update reshape for squeeze op...

@ashokkumarkannan1
Copy link
Contributor Author

@ashokkumarkannan1 how does this affect this PR #833? Here, one of the problems was that we called update reshape for squeeze op...

@dgolubovicTT This update_reshape_attr function is the first issue we faced even for XGLM model. After this fix only, we got this issue with `ttir.squeeze.

So Merging this PR won't be a blocker for this PR

@ashokkumarkannan1
Copy link
Contributor Author

@dgolubovicTT I will proceed with merging this PR. If you have any comments around this, Please let me know.

@ashokkumarkannan1 ashokkumarkannan1 merged commit 9895fde into main Jan 21, 2025
8 checks passed
@ashokkumarkannan1 ashokkumarkannan1 deleted the akannan/fix_update_reshape_attr branch January 21, 2025 15:34
pdeviTT pushed a commit that referenced this pull request Jan 21, 2025
…1038)

Fix: #816

This PR fixes the `update_reshape_attr called for a non-reshape
operation`. This is from commute_utils in erase inverse ops logic, where
this `update_reshape_attr` function called for squeeze, unsqueeze and
transpose ops. So added a condition to only call this for reshape ops.

Tested for Bart model and verified.

Log Before Fix -
[test_bart_without_fix.log](https://github.com/user-attachments/files/18486202/test_bart_without_fix.log)
Log After Fix -
[test_bart_with_fixt.log](https://github.com/user-attachments/files/18486199/test_bart_with_fixt.log)
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.

update_reshape_attr called for a non-reshape operation in whisper and swin model
3 participants