[Profiler] Unify global and thread local profiler lookup. #83894
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D38931521](https://our.internmc.facebook.com/intern/diff/D38931521/) [ghstack-poisoned]
🔗 Helpful links
✅ No Failures (5 Pending)As of commit 7a76909 (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. |
This was referenced Aug 23, 2022
This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D38931521](https://our.internmc.facebook.com/intern/diff/D38931521/) [ghstack-poisoned]
This was referenced Aug 24, 2022
added 3 commits
August 24, 2022 09:18
This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D38931521](https://our.internmc.facebook.com/intern/diff/D38931521/) [ghstack-poisoned]
This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D38931521](https://our.internmc.facebook.com/intern/diff/D38931521/) [ghstack-poisoned]
This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D38931521](https://our.internmc.facebook.com/intern/diff/D38931521/) [ghstack-poisoned]
added 2 commits
August 26, 2022 12:49
This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D38931521](https://our.internmc.facebook.com/intern/diff/D38931521/) [ghstack-poisoned]
This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D38931521](https://our.internmc.facebook.com/intern/diff/D38931521/) [ghstack-poisoned]
This was referenced Aug 30, 2022
robieta
added
ciflow/trunk
|
@pytorchbot merge --green |
|
@pytorchbot successfully started a merge job. Check the current status here. |
Merge failedReason: View failures on hud. Refusing to merge as mandatory check(s) pull failed for rule superuser. Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "circle-ci jobs have finished, but are not being detected" |
|
@pytorchbot successfully started a merge job. Check the current status here. |
|
Hey @robieta. |
|
@pytorchbot revert -m="Internal breakages " -c="ghfirst" |
|
@pytorchbot successfully started a revert job. Check the current status here. |
|
@robieta your PR has been successfully reverted. |
pytorchmergebot
added a commit
that referenced
this pull request
Sep 1, 2022
…3894)" This reverts commit c06a558. Reverted #83894 on behalf of https://github.com/mehtanirav due to [Internal breakages](https://www.internalfb.com/intern/sandcastle/job/13510799644553996/artifact/runsandcastle?selectedLines=990-990-7-65)
robieta
pushed a commit
that referenced
this pull request
Sep 7, 2022
…3894) This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D39326253](https://our.internmc.facebook.com/intern/diff/D39326253/) [ghstack-poisoned]
pytorchmergebot
pushed a commit
that referenced
this pull request
Sep 8, 2022
…3894) (#84668) This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D39326253](https://our.internmc.facebook.com/intern/diff/D39326253/) Pull Request resolved: #84668 Approved by: https://github.com/slgong-fb
robieta
pushed a commit
that referenced
this pull request
Sep 8, 2022
… local profiler lookup. (#83894)" This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D39326253](https://our.internmc.facebook.com/intern/diff/D39326253/) [ghstack-poisoned]
robieta
pushed a commit
that referenced
this pull request
Sep 8, 2022
…lookup. (#83894)" This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Differential Revision: [D39326253](https://our.internmc.facebook.com/intern/diff/D39326253/) [ghstack-poisoned]
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 9, 2022
…3894) (#84668) (#84668) Summary: This PR renames `ProfilerThreadLocalStateBase` to simply `ProfilerStateBase`, and adds `push`, `pop`, and `get` methods. `global` can be specified, or can be omitted for priority selection. In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt. A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding `ASSERT(pop())` on the caller side while allowing more complex handling for the kineto client interface. (Later PR.) Pull Request resolved: #84668 Approved by: https://github.com/slgong-fb Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/c06a5586f57c844fdc4a98e52f88e71f64dd54d2 Original Phabricator Test Plan: Unit tests should cover the TLS profiler case, and dyno integration test for on-demand. Reviewed By: chaekit, slgong-fb Differential Revision: D39326253 Pulled By: robieta fbshipit-source-id: 3400e2a1af29680c98322ed027da9be0b64a08b7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
SOFT_ASSERT#84275torch/testing/_internal/profiler_utils.pyand set_soft_assert_raiseson every test. #84274test/profilerfolder. #84273LibKinetoClient::stop()directly callProfilerStateBase::pop#83965RecordQueuemanage the lifetime ofPythonTracer. #83964PythonTracerBasetotorch/csrc/profiler/orchestration#83895This PR renames
ProfilerThreadLocalStateBaseto simplyProfilerStateBase, and addspush,pop, andgetmethods.globalcan be specified, or can be omitted for priority selection.In order to support this unification it was necessary to make a (mostly) non-throwing version of pop. The asserts around observer removal are intended to act as guard rails against multiple profilers trampling over each other. However on-demand wants to do exactly that because it wants to be able to preempt.
A hack would be to get the current observer and then only pop if an observer is found, but that would be prone to race conditions. By removing the asserts, we can preserve the old behavior by adding
ASSERT(pop())on the caller side while allowing more complex handling for the kineto client interface. (Later PR.)Differential Revision: D38931521