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

#14974: ttnn::empty Tensor creation API for MeshDevice #15191

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

omilyutin-tt
Copy link

@omilyutin-tt omilyutin-tt commented Nov 18, 2024

Ticket

#14974

Problem description

Extensions to Tensor creation APIs to support MeshDevice.

What's changed

Overload for ttnn::empty to support MeshDevice.

Minor formatting fixes / code comments.

Checklist

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • Device performance regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@omilyutin-tt omilyutin-tt changed the title Omilyutin/mesh creation Tensor creation APIs for MeshDevice Nov 18, 2024
const DataType& dtype,
const Layout& layout,
Device* device,
const MemoryConfig& memory_config) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@patrickroberts since I saw you recently touched these creation functions. fyi @eyonland @yan-zaretskiy

constexpr auto full =
    ttnn::decorators::register_operation_with_auto_launch_op<"ttnn::full", ttnn::operations::creation::Full>();
constexpr auto zeros = ttnn::decorators::register_operation<"ttnn::zeros", ttnn::operations::creation::Zeros>();
constexpr auto ones = ttnn::decorators::register_operation<"ttnn::ones", ttnn::operations::creation::Ones>();
constexpr auto empty = ttnn::decorators::register_operation<"ttnn::empty", ttnn::operations::creation::Empty>();

constexpr auto full_like =
    ttnn::decorators::register_operation_with_auto_launch_op<"ttnn::full_like", ttnn::operations::creation::FullLike>();
constexpr auto zeros_like =
    ttnn::decorators::register_operation<"ttnn::zeros_like", ttnn::operations::creation::ZerosLike>();
constexpr auto ones_like =
    ttnn::decorators::register_operation<"ttnn::ones_like", ttnn::operations::creation::OnesLike>();
constexpr auto empty_like =
    ttnn::decorators::register_operation<"ttnn::empty_like", ttnn::operations::creation::EmptyLike>();

constexpr auto arange =
    ttnn::decorators::register_operation_with_auto_launch_op<"ttnn::arange", ttnn::operations::creation::Arange>();

Some of these creation functions use register_operation_with_auto_launch_op and some use register_operation. For the ones that use register_operation I don't see see explicit calls to launch_op. Do we want to make all these invoke functions for these creation ops to be done under the scope of launch_op? It seems inconsistent right now?

Copy link
Contributor

Choose a reason for hiding this comment

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

When I updated those creation functions, I did not change whether they opted into auto_launch_op, that was determined by @arakhmati, and I don't have a good understanding of the criteria he used to decide that.

Copy link
Member

Choose a reason for hiding this comment

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

When op returns something else than a Tensor, for example a vector of tensors. In this case infra can't determine how many tensors to create or which to create so automatic decoration is not possible. In this case you must use register_operation and make an explicit call to launch_op inside operation's invoke.

If this is not handled correctly, operation won't be async even if async mode is enabled.

Good news, I am sure 6 months from now this wonderful infra should go away.

Copy link
Author

Choose a reason for hiding this comment

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

#10672 introduces the discrepancy, I think we can fix in a follow up? Under the hood, full/zeros/ones are ultimately the same; empty path is a bit simpler as it only requires an allocation.

Comment on lines +236 to +239
Tensor device_tensor = allocate_tensor_on_device(shape, dtype, layout, device, memory_config);
device_tensor.wait_for_tensor_metadata_populated();
device_tensor.wait_for_tensor_data_populated();
return device_tensor;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tt-asaigal what's the correct apis here to use. I don't think we want to call this in the worker thread?

@cfjchu
Copy link
Collaborator

cfjchu commented Nov 19, 2024

Also just following up from our convo to prioritize C++ APIs to add a few C++ unit tests.

@omilyutin-tt omilyutin-tt changed the title Tensor creation APIs for MeshDevice #14974: Tensor creation APIs for MeshDevice Nov 19, 2024
@omilyutin-tt
Copy link
Author

Also just following up from our convo to prioritize C++ APIs to add a few C++ unit tests.

Done. There are other tests that target different Tensor parameters, I've just added a basic test for the multi-device API.

@omilyutin-tt omilyutin-tt changed the title #14974: Tensor creation APIs for MeshDevice #14974: ttnn::empty Tensor creation API for MeshDevice Nov 19, 2024
@omilyutin-tt omilyutin-tt marked this pull request as ready for review November 19, 2024 18:42
@omilyutin-tt omilyutin-tt requested review from cfjchu, Denys88 and tt-asaigal and removed request for Denys88 November 19, 2024 18:43
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.

5 participants