Repository navigation
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/186263
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ You can merge normally! (5 Unrelated Failures)As of commit 8fb6c8a with merge base 29dbb32 ( FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
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. |
|
@claude review these changes |
|
Claude finished @jansel's task in 1m 37s —— View job Reviewing PR #186263: Increase small fake tensor constant limit
Review SummaryThis is a clean, well-motivated fix. Raising Feedback1. Duplicated constant — consider importing instead of syncing manually
from torch._subclasses.fake_tensor import (
CONSTANT_NUMEL_LIMIT,
FakeTensor,
FakeTensorMode,
...
)This avoids a future divergence if someone updates one but not the other. That said, if there's a deliberate reason for keeping them separate (e.g., historically different semantics or concern about coupling), the comment is acceptable — just flagging it as a potential maintenance hazard. 2. Performance regression acknowledged The PR description documents a ~1.87x slowdown on a synthetic microbenchmark that specifically hammers the constant-propagation path. Since this only affects tensors with ≤8 elements being lifted as constants (not general tracing throughput), it seems acceptable. Good that this was measured and disclosed. 3. Test quality
4. Minor: no The test at VerdictLooks good overall. The only actionable suggestion is consolidating the duplicated constant (item 1 above). The rest is solid. |
Fake tensor and proxy tensor tracing only kept tensor constants with one element. Shape tensors built from Python shape lists, such as torch.as_tensor([[64, 64], [32, 32]]), therefore lost their constant payload even though they are small. Later scalar uses of those values, including torch.linspace(..., steps=H), became data-dependent unbacked symbols and strict export failed with GuardOnDataDependentSymNode. Raise the small tensor constant propagation limit from 1 to 8 in both fake tensor and proxy tensor tracing. This keeps common small shape tensors constant while preserving a conservative cap. I chose this root-cause change over adding linspace or slicing special cases because maintainers pointed at more aggressive constant propagation and the same lost constant payload affects multiple downstream scalar consumers. The higher limit also exposed two expected follow-ups: tests that intentionally need dynamic fake values now create non-constant inputs, and Dynamo recompilation-count expectations are updated for the improved constant propagation. To avoid a real distributed regression, runtime DeviceMesh coordinate computation is excluded from fake/proxy constant propagation so compile-on-one-rank graphs do not bake rank-local literals. Fixes #136642 Generated by my agent Test Plan: - python test/export/test_export.py TestExport.test_strict_export_constant_props_small_shape_tensor - python test/test_fake_tensor.py FakeTensorTest.test_sparse_spdiags_dynamic_nnz_bound - python test/test_fake_tensor.py FakeTensorConstHandling - python test/dynamo/test_repros.py ReproTests.test_convert_boxes_to_pooler_format - python test/dynamo/test_dynamic_shapes.py DynamicShapesReproTests.test_convert_boxes_to_pooler_format_dynamic_shapes - python test/distributed/tensor/test_compile_on_one_rank.py TestCompileOnOneRank.test_compiled_rowwise_embedding_graph_consistency - git diff --check - lintrunner -a Benchmark Results: Synthetic FakeTensorMode benchmark, 1200 iterations creating a 4-element int64 tensor then add and multiply, 9 alternating reps with the limit toggled in-process. Old limit 1 median: 0.561588 s, 467.990 us/iter. New limit 8 median: 1.051484 s, 876.237 us/iter, 1.872x. This intentionally stresses the newly constant-propagated case; the change remains capped at tensors with at most 8 elements. ghstack-source-id: bc5dca5 Pull-Request: #186263
|
@claude review these changes |
|
My agent says I fixed the CI regressions from the higher small-constant limit. The update keeps runtime DeviceMesh coordinate computation out of fake/proxy constant propagation so compile-on-one-rank graphs do not bake rank-local literals, updates the dynamic spdiags test to use non-constant offsets, updates the Dynamo frame-count expectations, and deduplicates CONSTANT_NUMEL_LIMIT through the fake_tensor import. |
|
Claude finished @jansel's task in 1m 12s —— View job Reviewing PR #186263: Increase small fake tensor constant limit
Review SummaryThis is a well-motivated fix addressing the root cause of #136642. The change from limit 1 → 8 is conservative and directly solves shape tensors (built via Feedback1. Good: Consolidated The constant is now defined once in 2. The exclusion at and not func.name().startswith(
"device_mesh::_runtime_compute_coordinate_on_dim"
)This works but has a couple of concerns:
3. Test changes in The original test relied on 4. Frame count drops from 4 → 2 and op count changes. This is expected: with 5. Previously this test asserted that a 6. Performance note The 1.87x microbenchmark regression is acceptable given:
VerdictLooks good. The only minor suggestion is adding a brief comment at the |
Fake tensor and proxy tensor tracing only kept tensor constants with one element. Shape tensors built from Python shape lists, such as torch.as_tensor([[64, 64], [32, 32]]), therefore lost their constant payload even though they are small. Later scalar uses of those values, including torch.linspace(..., steps=H), became data-dependent unbacked symbols and strict export failed with GuardOnDataDependentSymNode. Raise the small tensor constant propagation limit from 1 to 8 in both fake tensor and proxy tensor tracing. This keeps common small shape tensors constant while preserving a conservative cap. I chose this root-cause change over adding linspace or slicing special cases because maintainers pointed at more aggressive constant propagation and the same lost constant payload affects multiple downstream scalar consumers. The higher limit also exposed expected follow-ups: tests that intentionally need dynamic fake values now create non-constant inputs, cache tests that specifically exercise dispatch-cache behavior clear the constant payload, and Dynamo recompilation-count expectations are updated for the improved constant propagation. To avoid a real distributed regression, runtime DeviceMesh coordinate computation is excluded from fake/proxy constant propagation so compile-on-one-rank graphs do not bake rank-local literals. Fixes #136642 Generated by my agent Test Plan: - python test/export/test_export.py TestExport.test_strict_export_constant_props_small_shape_tensor - python test/test_fake_tensor.py FakeTensorTest.test_sparse_spdiags_dynamic_nnz_bound - python test/test_fake_tensor.py FakeTensorConstHandling - python test/test_fake_tensor.py FakeTensorDispatchCache.test_cache_aten_index - python test/test_fake_tensor.py FakeTensorDispatchCache - python test/dynamo/test_repros.py ReproTests.test_convert_boxes_to_pooler_format - python test/dynamo/test_dynamic_shapes.py DynamicShapesReproTests.test_convert_boxes_to_pooler_format_dynamic_shapes - python test/distributed/tensor/test_compile_on_one_rank.py TestCompileOnOneRank.test_compiled_rowwise_embedding_graph_consistency - git diff --check - lintrunner test/test_fake_tensor.py torch/_subclasses/fake_tensor.py torch/fx/experimental/proxy_tensor.py - lintrunner -a Benchmark Results: Synthetic FakeTensorMode benchmark, 1200 iterations creating a 4-element int64 tensor then add and multiply, 9 alternating reps with the limit toggled in-process. Old limit 1 median: 0.561588 s, 467.990 us/iter. New limit 8 median: 1.051484 s, 876.237 us/iter, 1.872x. This intentionally stresses the newly constant-propagated case; the change remains capped at tensors with at most 8 elements. ghstack-source-id: 3151b0b Pull-Request: #186263
|
My agent says I fixed the new FakeTensorDispatchCache CI failures by making the cache-specific tests opt out of small-constant propagation explicitly. I also added the requested comments explaining that DeviceMesh coordinate ops must not be folded because their values are rank-dependent. |
Fake tensor and proxy tensor tracing only kept tensor constants with one element. Shape tensors built from Python shape lists, such as torch.as_tensor([[64, 64], [32, 32]]), therefore lost their constant payload even though they are small. Later scalar uses of those values, including torch.linspace(..., steps=H), became data-dependent unbacked symbols and strict export failed with GuardOnDataDependentSymNode. Raise the small tensor constant propagation limit from 1 to 8 in both fake tensor and proxy tensor tracing. This keeps common small shape tensors constant while preserving a conservative cap. I chose this root-cause change over adding linspace or slicing special cases because maintainers pointed at more aggressive constant propagation and the same lost constant payload affects multiple downstream scalar consumers. The higher limit also exposed expected follow-ups: tests that intentionally need dynamic fake values now create non-constant inputs, cache tests that specifically exercise dispatch-cache behavior clear the constant payload, and Dynamo recompilation-count expectations are updated for the improved constant propagation. To avoid a real distributed regression, runtime DeviceMesh coordinate computation is tagged as nondeterministic_seeded so existing fake/proxy constant propagation gates do not bake rank-local literals into compile-on-one-rank graphs. Fixes #136642 Generated by my agent Test Plan: - python test/export/test_export.py TestExport.test_strict_export_constant_props_small_shape_tensor - python test/test_fake_tensor.py FakeTensorTest.test_sparse_spdiags_dynamic_nnz_bound - python test/test_fake_tensor.py FakeTensorConstHandling - python test/test_fake_tensor.py FakeTensorDispatchCache.test_cache_aten_index - python test/test_fake_tensor.py FakeTensorDispatchCache - python test/dynamo/test_repros.py ReproTests.test_convert_boxes_to_pooler_format - python test/dynamo/test_dynamic_shapes.py DynamicShapesReproTests.test_convert_boxes_to_pooler_format_dynamic_shapes - python test/distributed/tensor/test_compile_on_one_rank.py TestCompileOnOneRank.test_compiled_rowwise_embedding_graph_consistency - python test/distributed/test_local_tensor.py TestLocalTensorWorld4.test_runtime_compute_coordinate_on_dim_per_rank_call_style_positional TestLocalTensorWorld4.test_runtime_compute_coordinate_on_dim_per_rank_call_style_kwargs - git diff --check - lintrunner test/test_fake_tensor.py torch/_subclasses/fake_tensor.py torch/fx/experimental/proxy_tensor.py - lintrunner -a Benchmark Results: Synthetic FakeTensorMode benchmark, 1200 iterations creating a 4-element int64 tensor then add and multiply, 9 alternating reps with the limit toggled in-process. Old limit 1 median: 0.561588 s, 467.990 us/iter. New limit 8 median: 1.051484 s, 876.237 us/iter, 1.872x. This intentionally stresses the newly constant-propagated case; the change remains capped at tensors with at most 8 elements. ghstack-source-id: 604ba81 Pull-Request: #186263
Fake tensor and proxy tensor tracing only kept tensor constants with one element. Shape tensors built from Python shape lists, such as torch.as_tensor([[64, 64], [32, 32]]), therefore lost their constant payload even though they are small. Later scalar uses of those values, including torch.linspace(..., steps=H), became data-dependent unbacked symbols and strict export failed with GuardOnDataDependentSymNode. Raise the small tensor constant propagation limit from 1 to 8 in both fake tensor and proxy tensor tracing. This keeps common small shape tensors constant while preserving a conservative cap. I chose this root-cause change over adding linspace or slicing special cases because maintainers pointed at more aggressive constant propagation and the same lost constant payload affects multiple downstream scalar consumers. The higher limit also exposed expected follow-ups: tests that intentionally need dynamic fake values now create non-constant inputs, cache tests that specifically exercise dispatch-cache behavior clear the constant payload, and Dynamo recompilation-count expectations are updated for the improved constant propagation. To avoid a real distributed regression, fake/proxy constant propagation now excludes runtime DeviceMesh coordinate computation so compile-on-one-rank graphs do not bake rank-local literals. Fixes #136642 Generated by my agent Test Plan: - python test/export/test_export.py TestExport.test_strict_export_constant_props_small_shape_tensor - python test/test_fake_tensor.py FakeTensorTest.test_sparse_spdiags_dynamic_nnz_bound - python test/test_fake_tensor.py FakeTensorConstHandling - python test/test_fake_tensor.py FakeTensorDispatchCache.test_cache_aten_index - python test/test_fake_tensor.py FakeTensorDispatchCache - python test/dynamo/test_repros.py ReproTests.test_convert_boxes_to_pooler_format - python test/dynamo/test_dynamic_shapes.py DynamicShapesReproTests.test_convert_boxes_to_pooler_format_dynamic_shapes - python test/distributed/tensor/test_compile_on_one_rank.py TestCompileOnOneRank.test_compiled_rowwise_embedding_graph_consistency - python test/distributed/test_local_tensor.py TestLocalTensorWorld4.test_runtime_compute_coordinate_on_dim_per_rank_call_style_positional TestLocalTensorWorld4.test_runtime_compute_coordinate_on_dim_per_rank_call_style_kwargs - git diff --check - lintrunner test/test_fake_tensor.py torch/_subclasses/fake_tensor.py torch/fx/experimental/proxy_tensor.py - lintrunner -a Rebase Update Test Plan: - git diff --check - lintrunner torch/_subclasses/fake_tensor.py torch/fx/experimental/proxy_tensor.py torch/distributed/_ops/device_mesh.py test/distributed/test_local_tensor.py - lintrunner -a - attempted targeted Python tests and import sanity check, but Python import hung in this workspace before executing test code Benchmark Results: Synthetic FakeTensorMode benchmark, 1200 iterations creating a 4-element int64 tensor then add and multiply, 9 alternating reps with the limit toggled in-process. Old limit 1 median: 0.561588 s, 467.990 us/iter. New limit 8 median: 1.051484 s, 876.237 us/iter, 1.872x. This intentionally stresses the newly constant-propagated case; the change remains capped at tensors with at most 8 elements. ghstack-source-id: b365b85 Pull-Request: #186263
|
@claude review these changes |
|
My agent says I rebased the PR onto current main and replaced the DeviceMesh handling with a narrow fake/proxy constant-folding exclusion instead of tagging the op as random. The torchbench failures Dr. CI highlighted are model load dependency failures on the old CI run rather than failures in this patch, so the updated CI should reclassify after the new head runs. |
|
Claude finished @jansel's task in 2m 25s —— View job Review: Increase small fake tensor constant limit
Main finding: the described tag-based fix is not in the current HEADIn your last review reply you said the The diff at HEAD doesn't reflect that. The string match is still there: # torch/_subclasses/fake_tensor.py:92-99
def can_constant_fold_through_op(func: torch._ops.OpOverload) -> bool:
if torch.Tag.nondeterministic_seeded in func.tags:
return False
# This op's result depends on the current rank, so folding one rank's value
# into a graph compiled for other ranks is incorrect.
if func.name() == "device_mesh::_runtime_compute_coordinate_on_dim":
return False
return TrueAnd the op is still registered with only # torch/distributed/_ops/device_mesh.py:10-14
torch.library.define(
"device_mesh::_runtime_compute_coordinate_on_dim",
"(Tensor full_mesh, int index) -> SymInt",
tags=torch.Tag.pt2_compliant_tag,
)The If you do go with the
|
|
My agent says this PR is being closed as part of a cleanup of open agentic PRs to focus review bandwidth on a smaller set; the linked issue remains open. |
Stack from ghstack (oldest at bottom):
Fake tensor and proxy tensor tracing only kept tensor constants with one element. Shape tensors built from Python shape lists, such as torch.as_tensor([[64, 64], [32, 32]]), therefore lost their constant payload even though they are small. Later scalar uses of those values, including torch.linspace(..., steps=H), became data-dependent unbacked symbols and strict export failed with GuardOnDataDependentSymNode.
Raise the small tensor constant propagation limit from 1 to 8 in both fake tensor and proxy tensor tracing. This keeps common small shape tensors constant while preserving a conservative cap. I chose this root-cause change over adding linspace or slicing special cases because maintainers pointed at more aggressive constant propagation and the same lost constant payload affects multiple downstream scalar consumers.
Fixes #136642
Generated by my agent
Test Plan:
lintrunner -a was also run and failed on an unrelated CLANGTIDY include-cycle in torch/csrc/distributed/autograd/functions/recvrpc_backward.h.
Benchmark Results:
Synthetic FakeTensorMode benchmark, 1200 iterations creating a 4-element int64 tensor then add and multiply, 9 alternating reps with the limit toggled in-process. Old limit 1 median: 0.561588 s, 467.990 us/iter. New limit 8 median: 1.051484 s, 876.237 us/iter, 1.872x. This intentionally stresses the newly constant-propagated case; the change remains capped at tensors with at most 8 elements.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @kadeng @chauhang @amjames @jataylo @azahed98