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

Removing skip_for_blackhole and fix some minor issues for blackhole conv2d #17222

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

skrsticTT
Copy link
Contributor

@skrsticTT skrsticTT commented Jan 28, 2025

Issues

#17221
#17216

Problem description

Enable conv2d blackhole tests - to begin with, remove the skips in tests/ttnn/unit_tests/operations/test_new_conv2d.py and provide a report about state of tests. Fix minor bugs described in issues.

What's changed

  • Removed @skip_for_blackhole() for tests that already run on wormhole_b0
  • Removed skip for tests that needs 8x8 grid for blackhole - it caused OOM for 8x7 grid on wormhole_b0, it should not be case on blackhole
  • Removed some unused imports and functions from
  • Changed alignment of an output tensor in calculate_L1_usage function
  • Removed fatals for num_cores in width sharded factory
  • Added workaround for shallow convs (from #17058, fyi @mywoodstock)
  • Changed act_block_w_div from 2 to 1 in some tests (this value is correlated with the number of input channels and grid size; 2 becomes invalid in these cases for blackhole due to the increased grid size)

State of blackhole tests after this changes

repro: pytest tests/ttnn/unit_tests/operations/test_new_conv2d.py

PASSED: 2301
FAILED: 8
99.7% pass rate

Of the tests that fail:
8 of them fail because of PCC error - #17226

Checklist

@skrsticTT skrsticTT self-assigned this Jan 28, 2025
@skrsticTT skrsticTT requested a review from a team as a code owner January 28, 2025 09:50
Copy link
Contributor

@pavlejosipovic pavlejosipovic left a comment

Choose a reason for hiding this comment

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

You will need to disable shallow convs for BH in auto shard codepath.

Copy link
Contributor

@pavlejosipovic pavlejosipovic left a comment

Choose a reason for hiding this comment

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

18 of them fail because of an assert conv2d_op_width_sharded_program_factory.cpp specify which assert is problematic and post new issue here.

@skrsticTT skrsticTT force-pushed the skrstic/enable-bh-tests branch from c5b8abc to 1733874 Compare January 29, 2025 18:06
Copy link
Contributor

@pavlejosipovic pavlejosipovic left a comment

Choose a reason for hiding this comment

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

Approved assuming pipelines are OK

@@ -879,7 +878,6 @@ def test_resnet50_conv_gs(


@skip_for_grayskull()
Copy link
Contributor

Choose a reason for hiding this comment

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

remove import for skip_blackhole

@skrsticTT skrsticTT force-pushed the skrstic/enable-bh-tests branch from 1733874 to 0a7e18c Compare January 30, 2025 17:37
@skrsticTT skrsticTT force-pushed the skrstic/enable-bh-tests branch from 0a7e18c to 8247e37 Compare January 31, 2025 10:08
@skrsticTT skrsticTT merged commit 25ee758 into main Jan 31, 2025
35 checks passed
@skrsticTT skrsticTT deleted the skrstic/enable-bh-tests branch January 31, 2025 13:48
nikileshx pushed a commit to nikileshx/tt-metal that referenced this pull request Feb 3, 2025
…onv2d (tenstorrent#17222)

### Issues
tenstorrent#17221
tenstorrent#17216


### Problem description
Enable conv2d blackhole tests - to begin with, remove the skips in
`tests/ttnn/unit_tests/operations/test_new_conv2d.py` and provide a
report about state of tests. Fix minor bugs described in issues.

### What's changed

- Removed `@skip_for_blackhole()` for tests that already run on
wormhole_b0
- Removed skip for tests that needs 8x8 grid for blackhole - it caused
OOM for 8x7 grid on wormhole_b0, it should not be case on blackhole
- Removed some unused imports and functions from 
- Changed alignment of an output tensor in `calculate_L1_usage` function
- Removed fatals for num_cores in width sharded factory
- Added workaround for shallow convs (from
[tenstorrent#17058](tenstorrent#17058), fyi
@mywoodstock)
- Changed act_block_w_div from 2 to 1 in some tests (this value is
correlated with the number of input channels and grid size; 2 becomes
invalid in these cases for blackhole due to the increased grid size)

### State of blackhole tests after this changes
repro: `pytest tests/ttnn/unit_tests/operations/test_new_conv2d.py`

PASSED: 2301
FAILED: 8 
99.7% pass rate

Of the tests that fail: 
8 of them fail because of PCC error - tenstorrent#17226

### Checklist
- [x] Post commit CI passes -
https://github.com/tenstorrent/tt-metal/actions/runs/13058074023
- [ ] Blackhole Post commit (if applicable)
- [x] Model regression CI testing passes (if applicable) -
https://github.com/tenstorrent/tt-metal/actions/runs/13070618785
- [ ] Device performance regression CI testing passes (if applicable)
- [ ] **(For models and ops writers)** Full [new
models](https://github.com/tenstorrent/tt-metal/actions/workflows/full-new-models-suite.yaml)
tests passes
- [ ] 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants