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

ttnn.embedding_bw inconsistent documentation regarding input tensors dtypes #13685

Closed
Tracked by #13795
amalbasaTT opened this issue Oct 10, 2024 · 3 comments
Closed
Tracked by #13795

ttnn.embedding_bw inconsistent documentation regarding input tensors dtypes #13685

amalbasaTT opened this issue Oct 10, 2024 · 3 comments
Labels
backward op_cat: embedding WH

Comments

@amalbasaTT
Copy link
Contributor

amalbasaTT commented Oct 10, 2024 •
edited
Loading

ttnn.embedding_bw doesn't support output and input gradient tensors having different dtypes. This bug/constraint should be put in the documentation.
.
To Reproduce
Steps to reproduce the behavior:
Sweep test for embedding_bw is located in 'tests/sweep_framework/sweeps/eltwise/binary_backward/embedding_bw/embedding_bw.py'

  1. Checkout branch amalbasaTT/backward_ops-sweeps-2 (soon to be merged to main)
git checkout amalbasaTT/backward_ops-sweeps-2
  1. Go to 'tests/sweep_framework/sweeps/eltwise/binary_backward/embedding_bw/embedding_bw.py'
  2. Remove invalidate_vector function
  3. Generate new parameter vectors and run the sweep test
python3 tests/sweep_framework/sweeps_parameter_generator.py --elastic cloud --module-name eltwise.binary_backward.embedding_bw.embedding_bw
python3 tests/sweep_framework/sweeps_runner.py --elastic cloud --module-name eltwise.binary_backward.embedding_bw.embedding_bw
  1. See the error. Results can be found on elastic cloud as explained here: https://github.com/tenstorrent/tt-metal/tree/main/tests/sweep_framework

Expected behavior
For vectors where output_dtype and grad_dtype are different, test will fail with the message "Output and input gradient tensors must have the same dtype"
@amalbasaTT amalbasaTT added bug Something isn't working op_cat: embedding WH backward op_cat: eltwise and removed bug Something isn't working labels Oct 10, 2024
@amahmudTT
Copy link
Contributor

@amalbasaTT For this bug do you mean that we are not supposed to allow different data types for output and input gradient tensors and we should exit with an error message ? Or do you mean different data types should be able to work together ?

@amalbasaTT
Copy link
Contributor Author

@amalbasaTT For this bug do you mean that we are not supposed to allow different data types for output and input gradient tensors and we should exit with an error message ? Or do you mean different data types should be able to work together ?

I mean that this constraint should be in the documentation. And I think that different data types should be supported. I fixed the issue text so that the description of the problem isn't so vague, my bad on that.

@eyonland eyonland mentioned this issue Oct 18, 2024
Open
@mcw-anasuya mcw-anasuya self-assigned this Nov 4, 2024
mcw-anasuya added a commit that referenced this issue Nov 4, 2024
@mcw-anasuya
#13685: Add note to the doc
7caffc9
@mcw-anasuya mcw-anasuya mentioned this issue Nov 4, 2024
Merged
3 tasks
mcw-anasuya added a commit that referenced this issue Nov 4, 2024
@mcw-anasuya
#13685: Add note to the doc
6852fe0
mcw-anasuya added a commit that referenced this issue Nov 4, 2024
@mcw-anasuya
#13685: Add note to the doc
482e902
mcw-anasuya added a commit that referenced this issue Nov 4, 2024
@mcw-anasuya
#13685: Move sweep test
bf88d4e
mcw-anasuya added a commit that referenced this issue Nov 5, 2024
@mcw-anasuya
#13685: Add note to the doc
4a51a5a
mcw-anasuya added a commit that referenced this issue Nov 5, 2024
@mcw-anasuya
#13685: Move sweep test
d31b214
mcw-anasuya added a commit that referenced this issue Nov 5, 2024
@mcw-anasuya
#13685: Move sweep test
ef9c84d
mcw-anasuya added a commit that referenced this issue Nov 8, 2024
@mcw-anasuya
#13685: Add note to the doc
1e80f63
mcw-anasuya added a commit that referenced this issue Nov 8, 2024
@mcw-anasuya
#13685: Move sweep test
b99c195
mcw-anasuya added a commit that referenced this issue Nov 11, 2024
@mcw-anasuya
#13685: Add note to the doc
475b2dd
mcw-anasuya added a commit that referenced this issue Nov 11, 2024
@mcw-anasuya
#13685: Move sweep test
030ff75
mcw-anasuya added a commit that referenced this issue Nov 11, 2024
@mcw-anasuya
#13685: Update backward embedding document (#14666)
fec3ebc 
### Ticket
#13685 

### Problem description
- ttnn.embedding_bw doesn't support output and input gradient tensors
having different dtypes.
- ttnn.embedding_bw is not an eltwise op. So, moving corresponding tests
outside eltwise folder.

### What's changed
- Updated document to add a note mentioning input and output gradients
must have the same datatype.
- Moved sweep test from
`tests/sweep_framework/sweeps/eltwise/binary_backward/embedding_bw/embedding_bw.py`
to `tests/sweep_framework/sweeps/embedding_bw/embedding_bw.py`
- Move unit test from from
`tests/ttnn/unit_tests/operations/eltwise/backward/test_backward_embedding.py`
to `tests/ttnn/unit_tests/operations/test_backward_embedding.py`


### Updated doc
<img width="1139" alt="Screenshot 2024-11-04 at 17 49 31"
src="https://github.com/user-attachments/assets/467fec4f-cb30-4ec9-ac26-9e59827ed45b">


### Checklist
- [x] [Post commit CI
passes](https://github.com/tenstorrent/tt-metal/actions/runs/11680989159)
- PASSED
- [x] [Run
Sweeps](https://github.com/tenstorrent/tt-metal/actions/runs/11678538301)
- PASSED
- [x] [Night fast dispatch
tests](https://github.com/tenstorrent/tt-metal/actions/runs/11678522765)
(same as main)
@mcw-anasuya
Copy link
Contributor

Updated document and merged to main. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backward op_cat: embedding WH
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@eyonland @mcw-anasuya @amalbasaTT @amahmudTT