[Profiler] Restructure inputs and capture TensorLists. #87825

robieta · 2022-10-26T23:41:41Z

Stack from ghstack (oldest at bottom):

This PR unifies and rationalizes some of the input representation in Result. The current approach of storing separate types in separate vectors is tedious for two types (Tensors and scalars), but would be even more annoying with the addition of TensorLists. A similar disconnection exists with sizes and strides which the user is also expected to zip with tensor_metadata.

I simplified things by moving inputs to a variant and moving sizes and strides into TensorMetadata. This also forced collection of sizes and strides in python tracer which helps to bring it in line with op profiling. Collection of TensorLists is fairly straightforward; InputOutputEncoder already has a spot for them (I actually collected them in the original TorchTidy prototype) so it was just a matter of plumbing things through.

Differential Revision: D40734451

This PR unifies and rationalizes some of the input representation in Result. The current approach of storing separate types in separate vectors is tedious for two types (Tensors and scalars), but would be even more annoying with the addition of TensorLists. A similar disconnection exists with sizes and strides which the user is also expected to zip with tensor_metadata. I simplified things by moving inputs to a variant and moving sizes and strides into TensorMetadata. This also forced collection of sizes and strides in python tracer which helps to bring it in line with op profiling. Collection of TensorLists is fairly straightforward; `InputOutputEncoder` already has a spot for them (I actually collected them in the original TorchTidy prototype) so it was just a matter of plumbing things through. Differential Revision: [D40734451](https://our.internmc.facebook.com/intern/diff/D40734451/) [ghstack-poisoned]

pytorch-bot · 2022-10-26T23:41:43Z

🔗 Helpful Links

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

📄 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 our office hours

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

✅ No Failures

As of commit da51841:
💚 Looks good so far! There are no failures yet. 💚

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

This PR unifies and rationalizes some of the input representation in Result. The current approach of storing separate types in separate vectors is tedious for two types (Tensors and scalars), but would be even more annoying with the addition of TensorLists. A similar disconnection exists with sizes and strides which the user is also expected to zip with tensor_metadata. I simplified things by moving inputs to a variant and moving sizes and strides into TensorMetadata. This also forced collection of sizes and strides in python tracer which helps to bring it in line with op profiling. Collection of TensorLists is fairly straightforward; `InputOutputEncoder` already has a spot for them (I actually collected them in the original TorchTidy prototype) so it was just a matter of plumbing things through. Differential Revision: [D40734451](https://our.internmc.facebook.com/intern/diff/D40734451/) [ghstack-poisoned]

slgong-fb

Overall LGTM. compilation error may be coming from python binding?

slgong-fb · 2022-10-27T17:12:35Z

torch/csrc/profiler/data_flow.cpp

+              c10::visit(raw_tensors, i);
            }
          },
-          [&](ExtraFields<EventType::Allocation>& alloc_op) {


we are not catching tensors from allocation calls any more?

We still are. [&](auto& i) { raw_tensors(i); })); forwards to RawTensors::operator()(ExtraFields<EventType::Allocation>&)

slgong-fb · 2022-10-27T17:45:12Z

torch/csrc/autograd/profiler_kineto.cpp

+            },
+            [&](const std::vector<TensorMetadata>&) {
+              shapes.emplace_back();
+              dtypes.emplace_back("TensorList");


n00b question - tensorlist is applied only to operator inputs?

Correct. That's the only place where we have to deal with nested structure.

This PR unifies and rationalizes some of the input representation in Result. The current approach of storing separate types in separate vectors is tedious for two types (Tensors and scalars), but would be even more annoying with the addition of TensorLists. A similar disconnection exists with sizes and strides which the user is also expected to zip with tensor_metadata. I simplified things by moving inputs to a variant and moving sizes and strides into TensorMetadata. This also forced collection of sizes and strides in python tracer which helps to bring it in line with op profiling. Collection of TensorLists is fairly straightforward; `InputOutputEncoder` already has a spot for them (I actually collected them in the original TorchTidy prototype) so it was just a matter of plumbing things through. Differential Revision: [D40734451](https://our.internmc.facebook.com/intern/diff/D40734451/) [ghstack-poisoned]

Pull Request resolved: #87825 This PR unifies and rationalizes some of the input representation in Result. The current approach of storing separate types in separate vectors is tedious for two types (Tensors and scalars), but would be even more annoying with the addition of TensorLists. A similar disconnection exists with sizes and strides which the user is also expected to zip with tensor_metadata. I simplified things by moving inputs to a variant and moving sizes and strides into TensorMetadata. This also forced collection of sizes and strides in python tracer which helps to bring it in line with op profiling. Collection of TensorLists is fairly straightforward; `InputOutputEncoder` already has a spot for them (I actually collected them in the original TorchTidy prototype) so it was just a matter of plumbing things through. ghstack-source-id: 171809541 Differential Revision: [D40734451](https://our.internmc.facebook.com/intern/diff/D40734451/)

robieta · 2022-10-27T22:39:24Z

compilation error may be coming from python binding?

@slgong-fb It was because I moved a ctor into a cpp file, so the class needed TORCH_API to link properly.

This PR unifies and rationalizes some of the input representation in Result. The current approach of storing separate types in separate vectors is tedious for two types (Tensors and scalars), but would be even more annoying with the addition of TensorLists. A similar disconnection exists with sizes and strides which the user is also expected to zip with tensor_metadata. I simplified things by moving inputs to a variant and moving sizes and strides into TensorMetadata. This also forced collection of sizes and strides in python tracer which helps to bring it in line with op profiling. Collection of TensorLists is fairly straightforward; `InputOutputEncoder` already has a spot for them (I actually collected them in the original TorchTidy prototype) so it was just a matter of plumbing things through. Differential Revision: [D40734451](https://our.internmc.facebook.com/intern/diff/D40734451/) [ghstack-poisoned]

robieta · 2022-11-08T20:04:02Z

@pytorchbot merge -g

pytorchmergebot · 2022-11-08T20:05:51Z

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours).

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

pytorchmergebot · 2022-11-08T20:31:16Z

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

robieta · 2022-11-08T21:42:46Z

@pytorchbot merge

pytorchmergebot · 2022-11-08T21:44:20Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

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

pytorchmergebot · 2022-11-08T21:44:26Z

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

robieta · 2022-11-08T21:47:03Z

@pytorchbot merge -f "test failure in merge job is unrelated. (Triton install)"

pytorchmergebot · 2022-11-08T21:48:36Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

This PR unifies and rationalizes some of the input representation in Result. The current approach of storing separate types in separate vectors is tedious for two types (Tensors and scalars), but would be even more annoying with the addition of TensorLists. A similar disconnection exists with sizes and strides which the user is also expected to zip with tensor_metadata. I simplified things by moving inputs to a variant and moving sizes and strides into TensorMetadata. This also forced collection of sizes and strides in python tracer which helps to bring it in line with op profiling. Collection of TensorLists is fairly straightforward; `InputOutputEncoder` already has a spot for them (I actually collected them in the original TorchTidy prototype) so it was just a matter of plumbing things through. Differential Revision: [D40734451](https://our.internmc.facebook.com/intern/diff/D40734451/) Pull Request resolved: pytorch#87825 Approved by: https://github.com/slgong-fb, https://github.com/chaekit

robieta requested review from aaronenyeshi, chaekit and slgong-fb October 26, 2022 23:49

robieta added the release notes: profiler release notes category label Oct 26, 2022

Taylor Robie added 2 commits October 26, 2022 16:56

slgong-fb approved these changes Oct 27, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 27, 2022

robieta mentioned this pull request Nov 8, 2022

[Profiler] E2E expecttests for category assignment #88653

Closed

robieta added the ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR label Nov 8, 2022

chaekit approved these changes Nov 8, 2022

View reviewed changes

pytorchmergebot added the Merged label Nov 8, 2022

pytorchmergebot closed this in 6e6f929 Nov 8, 2022

facebook-github-bot deleted the gh/robieta/147/head branch June 8, 2023 18:34

[Profiler] Restructure inputs and capture TensorLists. #87825

[Profiler] Restructure inputs and capture TensorLists. #87825

Uh oh!

Conversation

robieta commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Oct 26, 2022 • 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/87825

✅ No Failures

Uh oh!

slgong-fb left a comment

Choose a reason for hiding this comment

Uh oh!

slgong-fb Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

robieta Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

slgong-fb Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

robieta Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

robieta commented Oct 27, 2022

Uh oh!

robieta commented Nov 8, 2022

Uh oh!

pytorchmergebot commented Nov 8, 2022

Merge started

Uh oh!

pytorchmergebot commented Nov 8, 2022

Merge failed

Uh oh!

robieta commented Nov 8, 2022

Uh oh!

pytorchmergebot commented Nov 8, 2022

Merge started

Uh oh!

pytorchmergebot commented Nov 8, 2022

Merge failed

Uh oh!

robieta commented Nov 8, 2022

Uh oh!

pytorchmergebot commented Nov 8, 2022

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

robieta commented Oct 26, 2022 •

edited

Loading

pytorch-bot bot commented Oct 26, 2022 •

edited

Loading