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.

Sign up for GitHub

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

Jump to bottom

Change named attribute of reduce_avg to align with tt-mlir naming. #1101

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Change named attribute of reduce_avg to align with tt-mlir naming. #1101

merged 1 commit into from
Jan 27, 2025

Conversation

dgolubovicTT
Copy link
Contributor

@dgolubovicTT dgolubovicTT commented Jan 23, 2025 •
edited
Loading

Solves #1574

  • Avg pool2d decomposes to reduce_avg. Reduce avg op requires dim_arg attribute in ttir. Therefore, changing named attribute of reduce_avg from dim to dim_arg.
  • Edit unit tests and change xfail reason for test_avg_pool2d_resnet.

@dgolubovicTT dgolubovicTT self-assigned this Jan 23, 2025
@dgolubovicTT dgolubovicTT force-pushed the dgolubovic/fix-avgpool branch from fb97756 to e825b63 Compare January 23, 2025 17:27
@nvukobratTT
Copy link
Contributor

@mstojkovicTT Once we get your pass in place, let's see do we want to revert this change, and keep dim on our side while doing dim => dim_arg conversion in the pass directly.

TBH, dim_arg seems dusty to me 🥲

@dgolubovicTT
Copy link
Contributor Author

Agreed, this was just because we are currently obligated to align with attribute names with mlir...

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests492 ran429 passed63 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ☑️Skipped ⚠️Failed ❌️
TT-Forge-FE Tests550 ran468 passed79 skipped3 failed
TestResult
TT-Forge-FE Tests
pytest
test_nn.test_avg_pool2d❌ failure
test_resnet_inference❌ failure
test_resnet_unique_ops.test_avg_pool2d_resnet❌ failure

@dgolubovicTT
Copy link
Contributor Author

@nvukobratTT 3 tests XPASS on CI and fail with PCC error when I run them locally.

1. forge/test/mlir/operators/nn/test_nn.py::test_avg_pool2d
2. forge/test/mlir/resnet/test_resnet_inference.py::test_resnet_inference
3. forge/test/mlir/resnet/test_resnet_unique_ops.py::test_avg_pool2d_resnet

All pcc errors are due to avg pool2d op. It is strange why they all pass on CI.

@dgolubovicTT dgolubovicTT force-pushed the dgolubovic/fix-avgpool branch from e825b63 to 6286c1d Compare January 23, 2025 19:24
@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests550 ran471 passed79 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests492 ran429 passed63 skipped0 failed
TestResult
No test annotations available

@pilkicTT
Copy link
Contributor

pilkicTT commented Jan 24, 2025 •
edited
Loading

Aren't you making changes to the reduce_avg not avg_pool2d?

avg_pool2d might be lowered to reduce_avg? but this change is impacting all reduce_avg operation, so it would be good to clear that up in the commit message.

@dgolubovicTT
Copy link
Contributor Author

Aren't you making changes to the reduce_avg not avg_pool2d?

avg_pool2d might be lowered to reduce_avg? but this change is impacting all reduce_avg operation, so it would be good to clear that up in the commit message.

My bad, going back and forth between avg_pool test and decompose to reduce_avg... You are right, avg_pool is decomposed to reduce_avg, therefore the change... Will edit this.

@dgolubovicTT dgolubovicTT changed the title Change named attribute of avg pool2d to align with tt-mlir naming. Change named attribute of reduce_avg to align with tt-mlir naming. Jan 24, 2025
@dgolubovicTT
- Change named attribute for reduce avg op from dim to dim_arg to ali…
1fa75d9 
…gn with tt-mlir naming.

- Edit unit tests and change xfail reason for test_avg_pool2d_resnet.
@dgolubovicTT dgolubovicTT force-pushed the dgolubovic/fix-avgpool branch from 6286c1d to 1fa75d9 Compare January 27, 2025 11:54
@dgolubovicTT
Copy link
Contributor Author

dgolubovicTT commented Jan 27, 2025 •
edited
Loading

@nvukobratTT 3 tests XPASS on CI and fail with PCC error when I run them locally.

1. forge/test/mlir/operators/nn/test_nn.py::test_avg_pool2d
2. forge/test/mlir/resnet/test_resnet_inference.py::test_resnet_inference
3. forge/test/mlir/resnet/test_resnet_unique_ops.py::test_avg_pool2d_resnet

All pcc errors are due to avg pool2d op. It is strange why they all pass on CI.

Follow up: Tests were failing locally because of local environment. I tried to run them on other machine with clean build an they passed.

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests493 ran427 passed66 skipped0 failed
TestResult
No test annotations available

@github-actions GitHub Actions
Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests551 ran476 passed75 skipped0 failed
TestResult
No test annotations available

@dgolubovicTT dgolubovicTT merged commit f92e047 into main Jan 27, 2025
6 checks passed
@dgolubovicTT dgolubovicTT deleted the dgolubovic/fix-avgpool branch January 27, 2025 12:33
@mstojkovicTT
Copy link
Contributor

@mstojkovicTT Once we get your pass in place, let's see do we want to revert this change, and keep dim on our side while doing dim => dim_arg conversion in the pass directly.

TBH, dim_arg seems dusty to me 🥲

As #1117 is merged now, I will revert this change and add dim mapping to dim_arg in a separate PR :))

@mstojkovicTT mstojkovicTT mentioned this pull request Jan 29, 2025
Merged
1 task
mstojkovicTT added a commit that referenced this pull request Jan 30, 2025
@mstojkovicTT
Revert 'reduce_avg' attribute from 'dim_arg' to 'dim' and update op_m…
05094f3 
…apping (#1129)

### Problem description
The previous change in [PR
#1101](#1101) modified
the named attribute of `reduce_avg` from `dim` to `dim_arg` to align
with tt-mlir naming conventions. However, this change has been
identified as problematic and needs to be reverted.

### What's changed
This PR reverts the changes introduced in [PR
#1101](#1101), restoring
the named attribute of `reduce_avg` back to `dim`. Additionally, the
`op_mapping` has been updated from `dim` to `dim_arg` to ensure
compatibility with ttir.

### Checklist
- [x] New/Existing tests provide coverage for changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvukobratTT nvukobratTT nvukobratTT approved these changes

@pilkicTT pilkicTT Awaiting requested review from pilkicTT pilkicTT is a code owner

Assignees

@dgolubovicTT dgolubovicTT

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dgolubovicTT @nvukobratTT @pilkicTT @mstojkovicTT