Skip to content

[Profiler] Add queue depth computation#79993

Closed
davidchencsl wants to merge 12 commits intogh/davidchencsl/6/basefrom
gh/davidchencsl/6/head
Closed

[Profiler] Add queue depth computation#79993
davidchencsl wants to merge 12 commits intogh/davidchencsl/6/basefrom
gh/davidchencsl/6/head

Conversation

@davidchencsl
Copy link
Copy Markdown
Contributor

@davidchencsl davidchencsl commented Jun 21, 2022 •
edited
Loading

@davidchencsl
Add queue depth computation
f3f61c5 
[ghstack-poisoned]
@davidchencsl davidchencsl mentioned this pull request Jun 21, 2022
Closed
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jun 21, 2022 •
edited
Loading

🔗 Helpful links

✅ No Failures (19 Pending)

As of commit f4aed25 (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

davidchencsl added a commit that referenced this pull request Jun 22, 2022
@davidchencsl
Add queue depth computation
9014e87 
ghstack-source-id: f94911d
Pull Request resolved: #79993
@davidchencsl davidchencsl requested review from robieta and removed request for albanD and soulitzer June 22, 2022 00:03
davidchencsl added a commit that referenced this pull request Jun 22, 2022
@davidchencsl
Add queue depth computation
1699ba3 
ghstack-source-id: aea9e6a
Pull Request resolved: #79993
robieta
robieta requested changes Jun 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@robieta robieta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks quite good. Very natural extension of the original algorithm, good use of pythonic constructs like key= to gracefully handle the mixed types. Once you address the test comment this should be good to go.

Comment thread test/test_profiler.py Outdated
Comment thread torch/profiler/_utils.py Outdated
Comment thread torch/profiler/_utils.py Outdated
@davidchencsl davidchencsl mentioned this pull request Jun 22, 2022
Closed
@davidchencsl davidchencsl mentioned this pull request Jun 22, 2022
Closed
@davidchencsl davidchencsl changed the title Add queue depth computation [Profiler] Add queue depth computation Jun 22, 2022
@davidchencsl davidchencsl mentioned this pull request Jun 22, 2022
Closed
@davidchencsl
Update on "[Profiler] Add queue depth computation"
86a5f03 
Test Plan:
Add test in test_profiler.py

[ghstack-poisoned]
@davidchencsl davidchencsl mentioned this pull request Jun 23, 2022
Closed
@davidchencsl
Update on "[Profiler] Add queue depth computation"
5492b62 
Test Plan:
Add test in test_profiler.py

[ghstack-poisoned]
@davidchencsl
Update on "[Profiler] Add queue depth computation"
85b4de7 
Test Plan:
Add test in test_profiler.py

[ghstack-poisoned]
@davidchencsl
Update on "[Profiler] Add queue depth computation"
8c5bf29 
Test Plan:
Add test in test_profiler.py

[ghstack-poisoned]
@davidchencsl
Update on "[Profiler] Add queue depth computation"
bf8588d 
Test Plan:
Add test in test_profiler.py

[ghstack-poisoned]
robieta
robieta reviewed Jun 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@robieta robieta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, modulo the comment on the unit test. You've done a really good job managing the complexity that arose from mixing old and new style events.

Comment thread test/test_profiler.py Outdated
Comment thread test/test_profiler.py Outdated
Comment thread torch/autograd/__init__.py Outdated
@davidchencsl
Update on "[Profiler] Add queue depth computation"
f4aed25 
Test Plan:
Add test in test_profiler.py

[ghstack-poisoned]
robieta
robieta approved these changes Jun 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@robieta robieta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@davidchencsl
Copy link
Copy Markdown
Contributor Author

davidchencsl commented Jun 24, 2022

@pytorchmergebot merge

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

pytorchmergebot commented Jun 24, 2022

@pytorchbot successfully started a merge job. Check the current status here

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

pytorchmergebot commented Jun 24, 2022

Merge failed due to Command git -C /home/runner/actions-runner/_work/pytorch/pytorch cherry-pick -x 4dc8453c9235c6ce9f56507896a026b554d812c3 returned non-zero exit code 1

Auto-merging test/test_profiler.py
CONFLICT (content): Merge conflict in test/test_profiler.py
error: could not apply 4dc8453c92... Add queue depth computation
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Raised by https://github.com/pytorch/pytorch/actions/runs/2558058851

@davidchencsl
Copy link
Copy Markdown
Contributor Author

davidchencsl commented Jun 24, 2022

@pytorchmergebot merge

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

pytorchmergebot commented Jun 24, 2022

@pytorchbot successfully started a merge job. Check the current status here

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 24, 2022

Hey @davidchencsl.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit that referenced this pull request Jun 27, 2022
@davidchencsl @facebook-github-bot
[Profiler] Add queue depth computation (#79993) (#79993)
5ba9dc9 
Summary:
Pull Request resolved: #79993
Approved by: https://github.com/robieta

Test Plan:
contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/3a1e3e67c52b908ef39c7e48f6f61c818c40adec

Test plan from GitHub:
Add test in test_profiler.py

Reviewed By: atalman

Differential Revision: D37455671

Pulled By: davidchencsl

fbshipit-source-id: 3286707bf2ef1c8983ed7ffcc40c357408b5029b
@facebook-github-bot facebook-github-bot deleted the gh/davidchencsl/6/head branch June 28, 2022 14:17
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 25, 2026
@davidchencsl @pytorchmergebot
[Profiler] Add queue depth computation (pytorch#79993)
dad98d3 
Test Plan:
Add test in test_profiler.py
Pull Request resolved: pytorch#79993
Approved by: https://github.com/robieta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robieta robieta robieta approved these changes

Assignees

No one assigned

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@davidchencsl @facebook-github-bot @pytorchmergebot @robieta