[Tensor] Detect more cases of symbolic sizes/strides #123696

davidberard98 · 2024-04-10T01:28:35Z

Stack from ghstack (oldest at bottom):

-> [Tensor] Detect more cases of symbolic sizes/strides #123696

Previously, we'd just check has_symbolic_sizes_strides() to know whether a tensor has symbolic sizes or strides; if does, we skip some profiler logic. But sometimes has_symbolic_sizes_strides() returns false, but we do actually have symbolic sizes or strides.

So in this change, we add may_have_symbolic_sizes_strides() - which should never return false if the tensor has symbolic sizes and strides

Why not change has_symbolic_sizes_strides()? It seems like there's preexisting logic that assumes that "if has_symbolic_sizes_strides(), then we can assume that this tensor is guaranteed to have symbolic sizes or strides". In this case, we have python-implemented sizes or strides, which should follow a different code path.

Differential Revision: D55947660

Previously, we'd just check `has_symbolic_sizes_strides()` to know whether a tensor has symbolic sizes or strides; if does, we skip some profiler logic. But sometimes `has_symbolic_sizes_strides()` returns false, but we do actually have symbolic sizes or strides. So in this change, we add `may_have_symbolic_sizes_strides()` - which should never return false if the tensor has symbolic sizes and strides Why not change `has_symbolic_sizes_strides()`? It seems like there's preexisting logic that assumes that "if has_symbolic_sizes_strides(), then we can assume that this tensor is guaranteed to have symbolic sizes or strides". In this case, we have python-implemented sizes or strides, which should follow a different code path. Differential Revision: [D55947660](https://our.internmc.facebook.com/intern/diff/D55947660/) [ghstack-poisoned]

pytorch-bot · 2024-04-10T01:28:38Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/123696

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 23cd5b5 with merge base c3de2cc ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot · 2024-04-10T01:28:45Z

This pull request was exported from Phabricator. Differential Revision: D55947660

Previously, we'd just check `has_symbolic_sizes_strides()` to know whether a tensor has symbolic sizes or strides; if does, we skip some profiler logic. But sometimes `has_symbolic_sizes_strides()` returns false, but we do actually have symbolic sizes or strides. So in this change, we add `may_have_symbolic_sizes_strides()` - which should never return false if the tensor has symbolic sizes and strides Why not change `has_symbolic_sizes_strides()`? It seems like there's preexisting logic that assumes that "if has_symbolic_sizes_strides(), then we can assume that this tensor is guaranteed to have symbolic sizes or strides". In this case, we have python-implemented sizes or strides, which should follow a different code path. Differential Revision: [D55947660](https://our.internmc.facebook.com/intern/diff/D55947660/) [ghstack-poisoned]

facebook-github-bot · 2024-04-10T03:15:01Z

This pull request was exported from Phabricator. Differential Revision: D55947660

Pull Request resolved: #123696 Previously, we'd just check `has_symbolic_sizes_strides()` to know whether a tensor has symbolic sizes or strides; if does, we skip some profiler logic. But sometimes `has_symbolic_sizes_strides()` returns false, but we do actually have symbolic sizes or strides. So in this change, we add `may_have_symbolic_sizes_strides()` - which should never return false if the tensor has symbolic sizes and strides Why not change `has_symbolic_sizes_strides()`? It seems like there's preexisting logic that assumes that "if has_symbolic_sizes_strides(), then we can assume that this tensor is guaranteed to have symbolic sizes or strides". In this case, we have python-implemented sizes or strides, which should follow a different code path. ghstack-source-id: 221944770 Differential Revision: [D55947660](https://our.internmc.facebook.com/intern/diff/D55947660/)

aaronenyeshi

Looks great! Thanks for the fix and unit test!

aaronenyeshi · 2024-04-10T14:24:56Z

c10/core/TensorImpl.h

    return has_symbolic_sizes_strides_;
  }

+  bool may_have_symbolic_sizes_strides() const {


So in this change, we add may_have_symbolic_sizes_strides() - which should never return false if the tensor has symbolic sizes and strides

Nit: If this is strictly never, should it be named more strictly?

Hmm, in my opinion this naming more sense, e.g.:

tensor.definitely_has_symbolic_sizes_strides() -> if it returns true, we know that this tensor definitely has symbolic sizes/strides

tensor.may_have_symbolic_sizes_strides() -> if this returns true, then maybe the tensor has symbolic sizes/strides; but if this returns false, then we knwo this tensor definitely doesn't have symbolic sizes/strides.

I guess we could call it tensor.definitely_does_not_have_symbolic_sizes_strides() but that's a bit long...

I would agree that if we're saying we have 3 states now: definitely yes, maybe, definitely no, it would make sense for the existing has_symbolic_sizes_strides to be renamed to reflect what it is doing.

ok, named it "does_not_have_symbolic_sizes_strides" with a comment explaining (and inverted the return value)

Previously, we'd just check `has_symbolic_sizes_strides()` to know whether a tensor has symbolic sizes or strides; if does, we skip some profiler logic. But sometimes `has_symbolic_sizes_strides()` returns false, but we do actually have symbolic sizes or strides. So in this change, we add `may_have_symbolic_sizes_strides()` - which should never return false if the tensor has symbolic sizes and strides Why not change `has_symbolic_sizes_strides()`? It seems like there's preexisting logic that assumes that "if has_symbolic_sizes_strides(), then we can assume that this tensor is guaranteed to have symbolic sizes or strides". In this case, we have python-implemented sizes or strides, which should follow a different code path. Differential Revision: [D55947660](https://our.internmc.facebook.com/intern/diff/D55947660/) [ghstack-poisoned]

facebook-github-bot · 2024-04-10T22:00:02Z

This pull request was exported from Phabricator. Differential Revision: D55947660

Previously, we'd just check `has_symbolic_sizes_strides()` to know whether a tensor has symbolic sizes or strides; if does, we skip some profiler logic. But sometimes `has_symbolic_sizes_strides()` returns false, but we do actually have symbolic sizes or strides. So in this change, we add `may_have_symbolic_sizes_strides()` - which should never return false if the tensor has symbolic sizes and strides Why not change `has_symbolic_sizes_strides()`? It seems like there's preexisting logic that assumes that "if has_symbolic_sizes_strides(), then we can assume that this tensor is guaranteed to have symbolic sizes or strides". In this case, we have python-implemented sizes or strides, which should follow a different code path. Differential Revision: [D55947660](https://our.internmc.facebook.com/intern/diff/D55947660/) [ghstack-poisoned]

facebook-github-bot · 2024-04-10T22:06:27Z

This pull request was exported from Phabricator. Differential Revision: D55947660

Pull Request resolved: #123696 Previously, we'd just check `has_symbolic_sizes_strides()` to know whether a tensor has symbolic sizes or strides; if does, we skip some profiler logic. But sometimes `has_symbolic_sizes_strides()` returns false, but we do actually have symbolic sizes or strides. So in this change, we add `may_have_symbolic_sizes_strides()` - which should never return false if the tensor has symbolic sizes and strides Why not change `has_symbolic_sizes_strides()`? It seems like there's preexisting logic that assumes that "if has_symbolic_sizes_strides(), then we can assume that this tensor is guaranteed to have symbolic sizes or strides". In this case, we have python-implemented sizes or strides, which should follow a different code path. ghstack-source-id: 222066277 Differential Revision: [D55947660](https://our.internmc.facebook.com/intern/diff/D55947660/)

aaronenyeshi

LGTM, thanks for updating the name and adding comments!

soulitzer · 2024-04-11T05:37:06Z

c10/core/TensorImpl.h

+  // possible that has_symbolic_sizes_strides() returns false, but we do
+  // not have symbolic sizes/strides. This exists for the case of
+  // Nested Tensor python subclass, where the sizes are implemented in python
+  // (TODO: clean this up and just implement sizes in nested tensor without a


The naming still feels a little confusing to me since has_symbolic_sizes_strides() != !does_not_have_symbolic_sizes_strides() when the naming suggests that they should be inverses?

If we wanted to do something temporary to block NT (e.g. we were planning to remove this API anyway) and avoid renaming all callsites of has_symbolic_sizes_strides, maybe its just easiest to directly modify the callsite in execution_trace_observer.cpp to also check matches_policy(SizesStridesPolicy::CustomStrides)?

sorry, didn't see this after it was edited I think. I think matches_policy is protected so it's not accessible outside of TensorImpl. I guess we could also make that public, but I'm not sure if that's too different from this change?

soulitzer

Thanks for the quick fix!

My only comment is that the naming is still confusing imo and we're leaning on the comment to explain everything.

davidberard98 · 2024-04-11T15:18:51Z

@soulitzer recommendation for a new name? I'll land it now but can change the name

soulitzer · 2024-04-11T15:45:25Z

I don't think there's a better name unless we want to update all existing callsites too (which seems not worth it if this 3-state thing is temporary anyway)

my suggestion was just to avoid adding a new API altogether if we were just going to remove it later

(OR if we believe that subclasses in general can be in the situation where a instance can "sometimes" have symbolic shape, I think the naming from your comment here #123696 (comment) makes sense - though I cannot think of any examples of such subclasses)

facebook-github-bot · 2024-04-11T16:49:58Z

@pytorchbot merge -f 'Landed internally'

(Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally)

pytorchmergebot · 2024-04-11T16:51:45Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Previously, we'd just check `has_symbolic_sizes_strides()` to know whether a tensor has symbolic sizes or strides; if does, we skip some profiler logic. But sometimes `has_symbolic_sizes_strides()` returns false, but we do actually have symbolic sizes or strides. So in this change, we add `may_have_symbolic_sizes_strides()` - which should never return false if the tensor has symbolic sizes and strides Why not change `has_symbolic_sizes_strides()`? It seems like there's preexisting logic that assumes that "if has_symbolic_sizes_strides(), then we can assume that this tensor is guaranteed to have symbolic sizes or strides". In this case, we have python-implemented sizes or strides, which should follow a different code path. Differential Revision: [D55947660](https://our.internmc.facebook.com/intern/diff/D55947660/) Pull Request resolved: pytorch#123696 Approved by: https://github.com/aaronenyeshi, https://github.com/soulitzer

davidberard98 requested a review from aaronenyeshi as a code owner April 10, 2024 01:28

facebook-github-bot added the fb-exported label Apr 10, 2024

aaronenyeshi approved these changes Apr 10, 2024

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 10, 2024

aaronenyeshi added release notes: profiler release notes category topic: bug fixes topic category labels Apr 10, 2024

davidberard98 requested review from ezyang and soulitzer April 10, 2024 19:03

aaronenyeshi approved these changes Apr 10, 2024

View reviewed changes

davidberard98 mentioned this pull request Apr 11, 2024

[RFC] TensorImpl::has_symbolic_sizes_strides can return false when sizes() actually contains symbolic values #123698

Closed

soulitzer reviewed Apr 11, 2024

View reviewed changes

soulitzer approved these changes Apr 11, 2024

View reviewed changes

pytorchmergebot added the merging label Apr 11, 2024

pytorchmergebot closed this in 6b24ec4 Apr 11, 2024

pytorchmergebot added Merged and removed merging labels Apr 11, 2024

github-actions bot deleted the gh/davidberard98/286/head branch May 31, 2024 01:53

[Tensor] Detect more cases of symbolic sizes/strides #123696

[Tensor] Detect more cases of symbolic sizes/strides #123696

Uh oh!

Conversation

davidberard98 commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/123696

✅ No Failures

Uh oh!

facebook-github-bot commented Apr 10, 2024

Uh oh!

facebook-github-bot commented Apr 10, 2024

Uh oh!

aaronenyeshi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronenyeshi Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidberard98 Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

soulitzer Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

davidberard98 Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 10, 2024

Uh oh!

facebook-github-bot commented Apr 10, 2024

Uh oh!

aaronenyeshi left a comment

Choose a reason for hiding this comment

Uh oh!

soulitzer Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidberard98 Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

soulitzer left a comment

Choose a reason for hiding this comment

Uh oh!

davidberard98 commented Apr 11, 2024

Uh oh!

soulitzer commented Apr 11, 2024

Uh oh!

facebook-github-bot commented Apr 11, 2024

Uh oh!

pytorchmergebot commented Apr 11, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidberard98 commented Apr 10, 2024 •

edited

Loading

pytorch-bot bot commented Apr 10, 2024 •

edited

Loading

aaronenyeshi left a comment •

edited

Loading

aaronenyeshi Apr 10, 2024 •

edited

Loading

soulitzer Apr 11, 2024 •

edited

Loading