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

Change create_tt_tensor_from_py_data to use from_vector #16999

Closed
wants to merge 30 commits into from

Conversation

jjiangTT
Copy link

@jjiangTT jjiangTT commented Jan 22, 2025

Ticket

#16837

Problem description

Duplicate bespoke code in pytensor.cpp for bfloat handling, should probably use the tensor.cpp version for better maintainability.

What's changed

Call from_vector within pytensor.cpp instead of directly doing bfloat packing in pytensor.cpp, clean up from_span and create_owned_tensor_from_row_major_data helper method in tensor.cpp, add test cases to cover the new path.

Checklist

  • Post commit CI passes: https://github.com/tenstorrent/tt-metal/actions/runs/13038902373
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • Device performance regression CI testing passes (if applicable)
  • (For models and ops writers) Full new models tests passes
  • New/Existing tests provide coverage for changes: tt-metal/tests/ttnn/unit_tests/test_convert_python_tensor.py

@jjiangTT jjiangTT force-pushed the jjiang/16837-tensor_creation_and_conversion branch from f8e0eca to 4352603 Compare January 23, 2025 17:57
ttnn/cpp/pybind11/pytensor.cpp Outdated Show resolved Hide resolved
ttnn/cpp/pybind11/pytensor.cpp Outdated Show resolved Hide resolved
ttnn/cpp/pybind11/pytensor.cpp Show resolved Hide resolved
ttnn/cpp/pybind11/pytensor.cpp Outdated Show resolved Hide resolved
ttnn/cpp/pybind11/pytensor.cpp Outdated Show resolved Hide resolved
ttnn/cpp/pybind11/pytensor.cpp Outdated Show resolved Hide resolved
ttnn/cpp/pybind11/pytensor.cpp Outdated Show resolved Hide resolved
ttnn/cpp/ttnn/tensor/tensor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@bbradelTT bbradelTT left a comment

Choose a reason for hiding this comment

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

The files seem to have a bunch of un-related changes, such as adding ttnn/cpp/ttnn/operations/data_movement/permute/device/kernels/compute/transpose_xw_tiled.cpp which I'm pretty sure is in main.

Please update the PR appropriately to only have relevant changes.


## Model Bring-Up

- **Adding a Model** -
Copy link
Contributor

Choose a reason for hiding this comment

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

Will these sections be filled in as part of this PR?

Copy link
Author

@jjiangTT jjiangTT Jan 24, 2025

Choose a reason for hiding this comment

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

Sorry, it seems like I messed up a few rebases somewhere along the way by accidentally merging which added a bunch of files to my changelog. What's the best way to fix this? Git complains when I try to simply revert the relevant commits.
I guess I could just cherry pick all of my actual commits to main and then delete this branch when the time comes to merge?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cherry-pick this on top of main would work.

@jjiangTT jjiangTT requested a review from omilyutin-tt January 24, 2025 17:30
@jjiangTT jjiangTT requested a review from cfjchu January 27, 2025 22:42
@jjiangTT jjiangTT self-assigned this Jan 28, 2025
Copy link
Contributor

@omilyutin-tt omilyutin-tt left a comment

Choose a reason for hiding this comment

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

Nice! Just a few nit comments, the code is almost ready!

I do see your post commit is failing for 2 test cases though (the rest of failures seem to be due to timeouts):

ttnn/cpp/pybind11/pytensor.cpp Outdated Show resolved Hide resolved
@jjiangTT jjiangTT closed this Jan 30, 2025
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.

8 participants