Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Custom Operator Profiling Enhancement #15210

Merged
merged 38 commits into from Jun 28, 2019
Merged

Custom Operator Profiling Enhancement #15210

merged 38 commits into from Jun 28, 2019

Conversation

Zha0q1
Copy link
Contributor

@Zha0q1 Zha0q1 commented Jun 11, 2019 •
edited

Description

fix: #15241
I have implemented the new feature.
Need to add test cases.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@sandeep-krishnamurthy sandeep-krishnamurthy requested review from arcadiaphy and anirudh2290 and removed request for anirudh2290 and arcadiaphy June 11, 2019 05:50
@apache apache deleted a comment from Zha0q1 Jun 11, 2019
@vandanavk
Copy link
Contributor

@mxnet-label-bot add [Operator, pr-work-in-progress]

@marcoabreu marcoabreu added Operator pr-work-in-progress PR is still work in progress labels Jun 11, 2019
@access2rohit
Copy link
Contributor

There are no test cases associated with this new feature introduction. Please add test cases.

anirudh2290
anirudh2290 suggested changes Jun 11, 2019
View reviewed changes
src/engine/threaded_engine.cc Outdated Show resolved Hide resolved
src/profiler/custom_op_profiler.h Outdated Show resolved Hide resolved
src/profiler/profiler.h Outdated Show resolved Hide resolved
src/profiler/profiler.h Show resolved Hide resolved
@access2rohit
Copy link
Contributor

Even if the PR is WIP add the description and explain briefly what feature are you adding to MXNet profiler and show how it adds value over existing profiler

@Zha0q1 Zha0q1 mentioned this pull request Jun 13, 2019
Closed
arcadiaphy
arcadiaphy reviewed Jun 14, 2019
View reviewed changes
src/profiler/profiler.h Outdated Show resolved Hide resolved
src/profiler/profiler.h Show resolved Hide resolved
arcadiaphy
arcadiaphy reviewed Jun 14, 2019
View reviewed changes
src/profiler/profiler.h Outdated Show resolved Hide resolved
@Zha0q1 Zha0q1 closed this Jun 14, 2019
@Zha0q1 Zha0q1 reopened this Jun 14, 2019
@apeforest
Copy link
Contributor

Please add more description to this PR. What was done, what was tested, any limitationn

@Zha0q1
Copy link
Contributor Author

Zha0q1 commented Jun 17, 2019 •
edited

I have added a new test in test_profiler.py.
I also manually tested using Naive Engine; the result was the same as using ThreadedEngine. It did not seem like adding "os.environ['MXNET_ENGINE_TYPE'] = 'NaiveEngine'" to my new test case in test_profiler.py would work though. @arcadiaphy

@access2rohit
Copy link
Contributor

Can you more test cases like:
customOp1 + RegisteredOp + customOp2

Try to include python processing code in all the above cases

anirudh2290
anirudh2290 approved these changes Jun 24, 2019
View reviewed changes
Copy link
Member

@anirudh2290 anirudh2290 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 good. Lets wait for the CI. Also @arcadiaphy 's review constitutes a veto so I will wait for him to take another look before moving forward with this PR.

@anirudh2290
Copy link
Member

@arcadiaphy looks like your comments have been addressed. Can you please take another look ?

arcadiaphy
arcadiaphy reviewed Jun 27, 2019
View reviewed changes
src/profiler/profiler.h Outdated Show resolved Hide resolved
src/profiler/custom_op_profiler.h Show resolved Hide resolved
@anirudh2290 anirudh2290 merged commit 92fce90 into apache:master Jun 28, 2019
This was referenced Jun 28, 2019
Closed
Merged
access2rohit pushed a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 4, 2019
@Zha0q1
Custom Operator Profiling Enhancement (apache#15210)
eb60a47 
* working version

* style fix

* several fixes

* resolve issues in the comments

* revert to using thread-safe Get() for singleton class CustomOpProfiler

* indentation

* Now supports Naive Engine

* style fix

* tidiness

* tests added

* style fix

* add a new test case which has multiple custom ops

* testcases fix

* fix

* fix style

* minor naive engine fix

* simplify some branching logic

* better desing style

* fix

* fix

* fix

* fix

* fix

* fix

* add isprofiling check to onCustomStart

* fix

* rename dummy_wait

* fix conflict

* improve test

* fix

* fix test cases

* fix test cases

* fix testcases

* revert back to reduce overhead

* fix style

* Re-Trigger build

* rename var

* Re-Trigger build
access2rohit pushed a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 4, 2019
@Zha0q1
Custom Operator Profiling Enhancement (apache#15210)
0c7881f 
* working version

* style fix

* several fixes

* resolve issues in the comments

* revert to using thread-safe Get() for singleton class CustomOpProfiler

* indentation

* Now supports Naive Engine

* style fix

* tidiness

* tests added

* style fix

* add a new test case which has multiple custom ops

* testcases fix

* fix

* fix style

* minor naive engine fix

* simplify some branching logic

* better desing style

* fix

* fix

* fix

* fix

* fix

* fix

* add isprofiling check to onCustomStart

* fix

* rename dummy_wait

* fix conflict

* improve test

* fix

* fix test cases

* fix test cases

* fix testcases

* revert back to reduce overhead

* fix style

* Re-Trigger build

* rename var

* Re-Trigger build
access2rohit pushed a commit to access2rohit/incubator-mxnet that referenced this pull request Sep 6, 2019
Revert "Custom Operator Profiling Enhancement (apache#15210)"
c7147d9 
This reverts commit 92fce90.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apeforest apeforest apeforest left review comments

@access2rohit access2rohit access2rohit left review comments

@larroy larroy larroy approved these changes

@anirudh2290 anirudh2290 anirudh2290 approved these changes

@arcadiaphy arcadiaphy arcadiaphy approved these changes

Assignees
No one assigned
Labels
Operator pr-work-in-progress PR is still work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC] Custom Operator Profiling Enhancement
8 participants
@Zha0q1 @vandanavk @access2rohit @apeforest @anirudh2290 @larroy @arcadiaphy @marcoabreu