Fix max_width computation in _tensor_str._Formatter #126859
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/126859
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unrelated FailuresAs of commit 860b118 with merge base 62a49d9 ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@huydhn - would you please review this PR? |
colesbury
added
the
triaged
|
Hi, any updates on this ? |
|
Although the change makes sense to me, I think someone from core should be in a better position to have a final say about this as this change the tensor print output. I guess I can cc @malfet for a review. @malfet Plz help forward this to the correct code owners for their review if you have one in mind. |
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
|
Hi ! I saw the PR was automatically switch to stale. Is there any update on this ? |
|
Bumping, can someone please review this pull request? |
|
Would love to see this fix reviewed/merged! |
|
Same here, any news for this PR? @malfet |
malfet
added
release notes: python_frontend
|
@raphaelreme can you add some sort of a regression test to test_torch.py? |
|
@pytorchbot rebase |
|
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
|
Successfully rebased |
pytorchmergebot
force-pushed
the
rreme/fix_tensor_formatter
branch
from
3dc2d5e to
5a6e2fb
Compare
|
@malfet I simply updated the related test for sci_mode=False which was testing exactly for this weird behavior with leading 0s. Now the test correctly expects the formatter to use the minimal max_width even in sci_mode=False. Other tests are already written and should be unaffected by these changes. |
|
@pytorchbot rebase |
|
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
Previous version was not using PRINT_OPTS.sci_mode for the max_width computation but was using it for display leading to a weird discrepancy. Now, the code first checks if the display should be in sci_mode, then compute max_width
|
@pytorchbot merge -f "Signal is green this time around" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
@pytorchbot revert -m "broke trunk with test distributed/test_c10d_functional_native.py::CompileTest::test_inductor_all_reduce_single - RuntimeError: Expected to find buf7 = empty but did not find it" -c nosignal see |
|
@pytorchbot successfully started a revert job. Check the current status here. |
This reverts commit 1465757. Reverted #126859 on behalf of https://github.com/yangw-dev due to broke trunk with test distributed/test_c10d_functional_native.py::CompileTest::test_inductor_all_reduce_single - RuntimeError: Expected to find buf7 = empty but did not find it ([comment](#126859 (comment)))
|
@raphaelreme your PR has been successfully reverted. |
pytorch-bot
bot
dismissed
malfet’s stale review
This PR was reopened (likely due to being reverted), so your approval was removed. Please request another review.
|
@pytorchbot merge -f "Was reverted by mistake, actual root cause of failure is #147754 " |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: PR #126859 has not been reviewed yet |
Previous version of `torch._tensor_str._Formatter` was not using `PRINT_OPTS.sci_mode` for the `max_width` computation but was using it for the formatting of values leading to a weird discrepancy.
Now, the code first checks if it should be in sci_mode, then compute `max_width`
Here is an example to test the behavior:
```python
A = torch.tensor([10, 1e-1, 1e-2])
B = torch.tensor([10, 1e-1, 1e-1])
print("================= Default =================")
print(A, f"Formatter max_width: {torch._tensor_str._Formatter(A).max_width}")
print(B, f"Formatter max_width: {torch._tensor_str._Formatter(B).max_width}")
print("================= sci_mode=False =================")
with torch._tensor_str.printoptions(sci_mode=False):
print(A, f"Formatter max_width: {torch._tensor_str._Formatter(A).max_width}")
print(B, f"Formatter max_width: {torch._tensor_str._Formatter(B).max_width}")
print("================= sci_mode=True =================")
with torch._tensor_str.printoptions(sci_mode=True):
print(A, f"Formatter max_width: {torch._tensor_str._Formatter(A).max_width}")
print(B, f"Formatter max_width: {torch._tensor_str._Formatter(B).max_width}")
```
In the current version this prints:
```
================= Default =================
tensor([1.0000e+01, 1.0000e-01, 1.0000e-02]) Formatter max_width: 10
tensor([10.0000, 0.1000, 0.1000]) Formatter max_width: 7
================= sci_mode=False =================
tensor([ 10.0000, 0.1000, 0.0100]) Formatter max_width: 10
tensor([10.0000, 0.1000, 0.1000]) Formatter max_width: 7
================= sci_mode=True =================
tensor([1.0000e+01, 1.0000e-01, 1.0000e-02]) Formatter max_width: 10
tensor([1.0000e+01, 1.0000e-01, 1.0000e-01]) Formatter max_width: 7
```
On can see that in `sci_mode=False`, the values of A are prefixed with unneeded 0 and does not have the same `max_width` as B (It keeps the `max_width` from `sci_mode = None`)
Also in `sci_mode = True`, for B, the `max_width` is 7 but each value takes 10 chars... (But it is fine as the code that uses `max_width` do not rely much on it, but still, this is missleading)
After this commit, this will print
```
================= Default =================
tensor([1.0000e+01, 1.0000e-01, 1.0000e-02]) Formatter max_width: 10
tensor([10.0000, 0.1000, 0.1000]) Formatter max_width: 7
================= sci_mode=False =================
tensor([10.0000, 0.1000, 0.0100]) Formatter max_width: 7
tensor([10.0000, 0.1000, 0.1000]) Formatter max_width: 7
================= sci_mode=True =================
tensor([1.0000e+01, 1.0000e-01, 1.0000e-02]) Formatter max_width: 10
tensor([1.0000e+01, 1.0000e-01, 1.0000e-01]) Formatter max_width: 10
```
This also allows to align A with B for `sci_mode=False`.
Pull Request resolved: #126859
Approved by: https://github.com/malfet
This reverts commit 1465757. Reverted #126859 on behalf of https://github.com/yangw-dev due to broke trunk with test distributed/test_c10d_functional_native.py::CompileTest::test_inductor_all_reduce_single - RuntimeError: Expected to find buf7 = empty but did not find it ([comment](#126859 (comment)))
|
@malfet I think you have to approve again the PR before merging! |
|
@pytorchbot merge -f "We are at it again" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
9 participants
Previous version of
torch._tensor_str._Formatterwas not usingPRINT_OPTS.sci_modefor themax_widthcomputation but was using it for the formatting of values leading to a weird discrepancy.Now, the code first checks if it should be in sci_mode, then compute
max_widthHere is an example to test the behavior:
In the current version this prints:
On can see that in
sci_mode=False, the values of A are prefixed with unneeded 0 and does not have the samemax_widthas B (It keeps themax_widthfromsci_mode = None)Also in
sci_mode = True, for B, themax_widthis 7 but each value takes 10 chars... (But it is fine as the code that usesmax_widthdo not rely much on it, but still, this is missleading)After this commit, this will print
This also allows to align A with B for
sci_mode=False.