This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
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.
Custom Operator Profiling Enhancement #15210
Custom Operator Profiling Enhancement #15210
Changes from 37 commits
cb8479c
d549f5e
e57ebd5
ed1794e
5cf59fd
b9126a7
ed93849
cb21b2b
e21f18e
c908119
51b862a
36fcf62
e553a38
9f1275d
0fd48f5
9820bac
89de9f4
3e97083
5b3ab96
b40b432
5fdaa56
693fa23
602b3a2
c4a2355
73b764b
afce2f0
08a4d54
51f5b1a
4e5e14c
e9a7982
79b4e00
ba22dda
2db1243
0c4294b
c939067
0f5e47e
8aac853
32f5eaf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Custom profiler doesnt respect start and stop for profiling currently. This means that irrespective of profiler start stop this will profile. You can take a look at device storage profiler for doing similar checks -> mode should be kSymbolic | kImperative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check profiler->IsProfiling(profiler::Profiler::kImperative), which was the same check used for profiling other operators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay , what happens for symbolic case ? does it fallback to old custom op display for profiling or does it not display anything at all for custom op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently if you set profile_imperative to False, then you will have no profiling events for pure_python, aka the forward and backward callbacks. This is the case for both symbolic and imperative mode. I think this should be fine though. Considering that within custom operators,
users are mostly calling regular NDArray operators in imperative mode, I think if they ever want to peek inside custom operators, they must set profile_imperative to True.
To sum up, if profile_imperative == True, uses would have all the fine grained profiling events including pure_python and sub-operators. If profile_imperative == False, they would nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think profile_symbolic should also have this info, otherwise users may have a symbolic model with custom op but won't see any info on custom op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this line can be merged with https://github.com/apache/incubator-mxnet/pull/15210/files#diff-b2c0746deeb2e04ea9cdd8139d6b30b2R1159
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean use a tertiary operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will keep it as a separate line. The reason is that we want to set the domain and category in the same if else block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case you are calling as_task_ twice with the second call overwrites the previous one. Using a ternary operator during class initialization seems cleaner to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that because line 1165 and line 1166 are basically doing the same thing, except for different objects, we should not move 1165 up. Also, I think keeping the initialization list concise is also good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a good design style is actually keep the body in constructor simple. In fact, if you do
Then you don't even need a if/else branch for SetCategories. You can simply write:
because the domain_ here is already customer_domain in your if case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's true. domain_ is defined as "ProfileDomain ProfileOperator::domain_("operator");" in profiler.cc, line 44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
domain_ is a global variable, not a member variable of ProfileOperator