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

We need a good way to log Tensor shapes in portable ops' error messages #7902

Closed
swolchok opened this issue Jan 23, 2025 · 3 comments
Closed

We need a good way to log Tensor shapes in portable ops' error messages #7902

swolchok opened this issue Jan 23, 2025 · 3 comments
Assignees
@swolchok
Labels
module: user experience Issues related to reducing friction for users triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Milestone
0.6.0

Comments

@swolchok
Copy link
Contributor

swolchok commented Jan 23, 2025 •
edited by pytorch-bot bot
Loading

🚀 The feature, motivation and pitch

Currently, when we complain about tensors' shapes, we don't include them in the log message because it's not obvious or convenient how to do this. I think we can write a shape_to_string function that uses an extremely finite amount of stack space (kTensorDimensionLimit * 4 digits per dim + commas, spaces, and parentheses) and use that to do this logging. (@dbort , how big an array can I put on the stack on a prototypical embedded system these days, and/or do you have any other ideas for how to do this?)

Alternatives

No response

Additional context

Motivating example: #7748 (comment)

RFC (Optional)

No response

cc @mergennachin @byjlw
@swolchok swolchok mentioned this issue Jan 23, 2025
Closed
@swolchok swolchok self-assigned this Jan 24, 2025
This was referenced Jan 24, 2025
Merged
Merged
swolchok added a commit that referenced this issue Jan 24, 2025
@swolchok
Include tensor shapes in get_broadcast_target_size error message
5ea0340 
This is the motivating example for #7902.

Test Plan: Injected failure to new broadcast_test and saw shapes in error message.

ghstack-source-id: 7f41462480101d4c7d781de536ce9784de4d5ad7
ghstack-comment-id: 2613189501
Pull Request resolved: #7944
@swolchok swolchok added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 27, 2025
swolchok added a commit that referenced this issue Jan 29, 2025
@swolchok
Include tensor shapes in get_broadcast_target_size error message
a748b03 
This is the motivating example for #7902.

Test Plan: Injected failure to new broadcast_test and saw shapes in error message.

ghstack-source-id: 3802db8e835a0047322b1cba54b7e81e4c426ba5
ghstack-comment-id: 2613189501
Pull Request resolved: #7944
swolchok added a commit that referenced this issue Jan 31, 2025
@swolchok
Include tensor shapes in get_broadcast_target_size error message
be76ad3 
This is the motivating example for #7902.

Test Plan: Injected failure to new broadcast_test and saw shapes in error message.

ghstack-source-id: c5997670c116265e33e964bc88baef54d5885bad
ghstack-comment-id: 2613189501
Pull Request resolved: #7944
swolchok added a commit that referenced this issue Feb 5, 2025
@swolchok
Include tensor shapes in get_broadcast_target_size error message
115da12 
This is the motivating example for #7902.

Test Plan: Injected failure to new broadcast_test and saw shapes in error message.

ghstack-source-id: 6ae4789fa101cbcdedd316490e83a951460479bd
ghstack-comment-id: 2613189501
Pull Request resolved: #7944
swolchok added a commit that referenced this issue Feb 6, 2025
@swolchok
Add function to stringify tensor shapes (#7943)
4a1bf29 
First step toward fixing #7902
@swolchok
Copy link
Contributor Author

swolchok commented Feb 6, 2025

I just found sizes_to_string, used in one place: https://github.com/search?q=repo%3Apytorch%2Fexecutorch%20sizes_to_string&type=code
Should replace this with tensor_shape_to_string from #7943.

swolchok added a commit that referenced this issue Feb 7, 2025
@swolchok
Include tensor shapes in get_broadcast_target_size error message (#7944)
ef30176 
This is the motivating example for #7902.

Test Plan: Injected failure to new broadcast_test and saw shapes in error message.
@swolchok swolchok mentioned this issue Feb 7, 2025
Merged
swolchok added a commit that referenced this issue Feb 7, 2025
@swolchok
Use tensor_shape_to_c_string for error in check_mask_indices
e96a6b7 
Rolling out for #7902

ghstack-source-id: db14c70dfafc167611004d7842325d3f6e0ebbde
ghstack-comment-id: 2643854240
Pull Request resolved: #8314
@swolchok swolchok mentioned this issue Feb 7, 2025
Merged
swolchok added a commit that referenced this issue Feb 7, 2025
@swolchok
Use tensor_shape_to_c_string for error in check_glu_args
56cddf4 
Rolling out for #7902

ghstack-source-id: 25379114bb1af9572c61d4f9673805a471ea99c6
ghstack-comment-id: 2643883485
Pull Request resolved: #8316
@swolchok
Copy link
Contributor Author

swolchok commented Feb 7, 2025

Closing because we now have a good way. Systematically fixing error messages seems covered by #8273

@swolchok swolchok closed this as completed Feb 7, 2025
swolchok added a commit that referenced this issue Feb 12, 2025
@swolchok
Use tensor_shape_to_c_string for error in check_mask_indices (#8314)
ee7d388 
Rolling out for #7902
swolchok added a commit that referenced this issue Feb 13, 2025
@swolchok
Use tensor_shape_to_c_string for error in check_glu_args
9a14af8 
Rolling out for #7902

ghstack-source-id: 1bf9048e87efda09a174c2161cd09990a6d89a92
ghstack-comment-id: 2643883485
Pull Request resolved: #8316
@mergennachin mergennachin added the module: user experience Issues related to reducing friction for users label Feb 13, 2025
@mergennachin mergennachin added this to the 0.6.0 milestone Feb 13, 2025
@github-project-automation github-project-automation bot moved this to To triage in ExecuTorch DevX Feb 13, 2025
@mergennachin mergennachin moved this from To triage to Done in ExecuTorch DevX Feb 13, 2025
@mergennachin
Copy link
Contributor

Thank you @swolchok -- this is a good user experience improvement!

swolchok added a commit that referenced this issue Feb 13, 2025
@swolchok
Use tensor_shape_to_c_string for error in check_glu_args (#8316)
f8e3676 
Rolling out for #7902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@swolchok swolchok

Labels
module: user experience Issues related to reducing friction for users triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
ExecuTorch DevX
Status: Done
Milestone
0.6.0
Development

No branches or pull requests
2 participants
@swolchok @mergennachin