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

#0: Improve error messages in Tensor storage classes #18321

Open

jjiangTT wants to merge 8 commits into main from jjiang/spec_error_improvements

Contributor

jjiangTT commented Feb 25, 2025 •

edited

Loading

Ticket

Link to Github Issue: N/A

Problem description

An error was a little unclear and potentially misleading and some breaking errors were marked as assert rather than fatal.

What's changed

Reworded and changed a spec assert to a TT_FATAL making the problem more clear for users who run into this error in the future.
Changed some buffer checks to FATAL.

Checklist

All post commit CI passes: https://github.com/tenstorrent/tt-metal/actions/runs/13537003520/job/37858354651
Blackhole Post commit CI passes (if applicable)
Model regression CI passes (if applicable)
Device performance regression CI passes (if applicable)
(For models and ops writers) Full new models tests CI passes (if applicable)
New/Existing tests provide coverage for changes

jjiangTT requested review from cfjchu, ayerofieiev-tt and dmakoviichuk-tt as code owners

February 25, 2025 23:10

dmakoviichuk-tt reviewed

View reviewed changes

ttnn/cpp/ttnn/tensor/storage.hpp Outdated

    
            @@ -200,7 +200,7 @@ struct MultiDeviceHostStorage {
          
                  TensorSpec get_tensor_spec(int spec_index) const {

                      std::lock_guard<std::mutex> lock(mtx);

                      TT_ASSERT(spec_index < specs.size(), "Buffer not found for device {}", spec_index);

                      TT_ASSERT(spec_index < specs.size(), "Spec for device {} not found in spec list, were buffer and spec added?", spec_index);

Contributor

dmakoviichuk-tt Feb 26, 2025

should it be the exception?

Contributor

omilyutin-tt Feb 26, 2025

I think this can be TT_FATAL. I'd also argue "were buffer and spec added?" is more confusing for the end user to see than just a failing assertion.

Raises more questions unnecessarily.

Contributor Author

jjiangTT Feb 26, 2025

Assuming by exception Denys means a TT_THROW, I personally think a FATAL would be more clear. Both would be preferable to the existing assert though.

jjiangTT changed the title ~~jjiang/spec error improvements~~ jjiang/clarify-spec-assert

jjiangTT changed the title ~~jjiang/clarify-spec-assert~~ jjiang/clarify_spec_assert

jjiangTT changed the title ~~jjiang/clarify_spec_assert~~ jjiang/spec_error_improvements

jjiangTT force-pushed the jjiang/spec_error_improvements branch from f87408a to 3b83b09 Compare

February 26, 2025 19:25

jjiangTT requested review from omilyutin-tt and dmakoviichuk-tt

February 26, 2025 19:28

omilyutin-tt reviewed

View reviewed changes

ttnn/cpp/ttnn/tensor/storage.hpp

    
            @@ -200,7 +200,7 @@ struct MultiDeviceHostStorage {
          
                  TensorSpec get_tensor_spec(int spec_index) const {

                      std::lock_guard<std::mutex> lock(mtx);

                      TT_ASSERT(spec_index < specs.size(), "Buffer not found for device {}", spec_index);

                      TT_FATAL(spec_index < specs.size(), "Spec for device {} not found in spec list", spec_index);

Contributor

omilyutin-tt Feb 26, 2025

I think you can make the same change in other places in the file, for consistency

Contributor

omilyutin-tt Feb 26, 2025

This looks good!

jjiangTT force-pushed the jjiang/spec_error_improvements branch from 3b83b09 to 58e542a Compare

February 27, 2025 15:47

jjiangTT requested a review from omilyutin-tt

February 27, 2025 15:55

omilyutin-tt reviewed

View reviewed changes

ttnn/cpp/ttnn/tensor/storage.hpp Outdated

@@ @@ -325,7 +325,7 @@ struct MultiDeviceStorage { @@
                   inline std::shared_ptr<Buffer> get_buffer_for_device(IDevice* device) const {
                       std::lock_guard<std::mutex> lock(buffer_mtx);
-                      TT_ASSERT(buffers.find(device->id()) != buffers.end(), "Buffer not found for device {}", device->id());
+                      TT_FATAL(buffers.find(device->id()) != buffers.end(), "Buffer not found for device {}", device->id());

Contributor

omilyutin-tt Feb 27, 2025

For this one and below, you are performing redundant lookups in an unordered map. This involves hashing the key, and potentially iterating over a linked list of keys that hashes to the same value.

To avoid this, you can do:

auto buffer_it = buffers.find(device->id();
TT_FATAL(buffer_it != buffers.end(), ...)
// Perhaps we can make this TT_FATAL as well, but let's leave for now.
TT_ASSERT(...)
return buffer_it->second;

Contributor

omilyutin-tt commented Feb 27, 2025

Also jjiang/spec_error_improvements can be renamed to #0: Improve error messages in Tensor storage classes (#0 as there is no issue number associated with this clean up)

jjiangTT changed the title ~~jjiang/spec_error_improvements~~ #0: Improve error messages in Tensor storage classes

jjiangTT force-pushed the jjiang/spec_error_improvements branch from 0b7f743 to 8a9e4b1 Compare

March 3, 2025 17:26

jjiangTT requested a review from omilyutin-tt

March 3, 2025 17:27

omilyutin-tt reviewed

View reviewed changes

ttnn/cpp/ttnn/tensor/storage.hpp Outdated

                   }
                   inline std::shared_ptr<Buffer>& get_buffer_for_device(IDevice* device) {
                       std::lock_guard<std::mutex> lock(buffer_mtx);
-                      TT_ASSERT(buffers.find(device->id()) != buffers.end(), "Buffer not found for device {}", device->id());
+                      TT_FATAL(buffers.find(device->id()) != buffers.end(), "Buffer not found for device {}", device->id());

Contributor

omilyutin-tt Mar 3, 2025

Do the same here and below in get_tensor_spec_for_device

omilyutin-tt reviewed

View reviewed changes

ttnn/cpp/ttnn/tensor/storage.hpp Outdated

                       TT_ASSERT(
-                          buffers.at(device->id())->device() == device,
+                          buffer_it->device() == device,

Contributor

omilyutin-tt Mar 3, 2025

buffer_it->second->device() I believe? TT_ASSERT is only compiled in debug mode, try to build with --debug flag.

jjiangTT force-pushed the jjiang/spec_error_improvements branch 2 times, most recently from cf9d42e to 4c9ed77 Compare

March 3, 2025 19:20

jjiangTT requested a review from omilyutin-tt

March 3, 2025 22:45

jjiangTT force-pushed the jjiang/spec_error_improvements branch 5 times, most recently from 8eea513 to 81d6f70 Compare

March 6, 2025 18:47

omilyutin-tt approved these changes

View reviewed changes


          Clarify error message

ada5273

jjiangTT added 7 commits

March 6, 2025 23:15


          improve error clarification

39b8ce1


          change assert to a fatal

1699e7d


          Change asserts to fatals where reasonable

f485fd1


          Search buffer for device id once only

530e024


          fix pointer error

ee92213


          fix pair access

d4227ab


          fix ->second error, replace other redundant searches

4eeb969

jjiangTT force-pushed the jjiang/spec_error_improvements branch from 81d6f70 to 4eeb969 Compare

March 6, 2025 23:15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

omilyutin-tt omilyutin-tt approved these changes

cfjchu Awaiting requested review from cfjchu cfjchu is a code owner

ayerofieiev-tt Awaiting requested review from ayerofieiev-tt ayerofieiev-tt is a code owner

dmakoviichuk-tt Awaiting requested review from dmakoviichuk-tt dmakoviichuk-tt is a code owner

Labels

None yet