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

Implement remaining nn_basic ops in opperf #17456

Merged
merged 53 commits into from
Feb 19, 2020
Merged

Implement remaining nn_basic ops in opperf #17456

merged 53 commits into from
Feb 19, 2020

Conversation

connorgoggins
Copy link
Contributor

@connorgoggins connorgoggins commented Jan 28, 2020 •
edited
Loading

Description

This PR serves to implement the remaining operators from the nn_basic category in opperf.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • M benchmark/opperf/nd_operations/nn_basic_operators.py
  • M benchmark/opperf/rules/default_params.py
  • M benchmark/opperf/utils/benchmark_utils.py
  • M benchmark/opperf/utils/op_registry_utils.py

Comments

Tested on c5.18xl-ubuntu 16.04 and Mac OS with:

  1. run_performance_test on individual ops
  2. Checkout branch and call function run_nn_basic_operators_benchmarks
  3. Checkout branch and run opperf.py (full run of all ops)

@apeforest @access2rohit

ChaiBapchya
ChaiBapchya suggested changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

As discussed offline
Not a good practice to add duplicate (redundant code)
30-40% of the lines can be taken care of with a function. So lets get that across this entire file.

Thanks for the contribution again!

@apeforest apeforest mentioned this pull request Jan 29, 2020
Open
@connorgoggins
Copy link
Contributor Author

connorgoggins commented Jan 30, 2020 •
edited
Loading

Group of operator test - all NN Basic ops (GPU)
Group of operator test - all NN Basic ops (CPU)

Full OpPerf test (GPU)
Full OpPerf test (CPU)

*Note: couldn't run SoftmaxOutput op backwards on GPU - see previously documented issue and couldn't run im2col either forwards or backwards on GPU - see new documented issue. In both cases, simply switching the context from cpu to gpu leads the op to fail.

@ChaiBapchya
Copy link
Contributor

I was able to run GPU tests with topk - https://gist.github.com/ChaiBapchya/fac7310f7d167d1361854451e7daa342

That shouldn't be a problem. Please confirm.

@access2rohit
Copy link
Contributor

access2rohit commented Jan 31, 2020 •
edited
Loading

Can we change the logic here to ctx == mx.cpu() or op not in gpu_disabled_ops for readability?

Address the quoted and profiler comment made by @apeforest. Rest LGTM !

@connorgoggins
Copy link
Contributor Author

Updated Full OpPerf GPU Results with topk perf: https://gist.github.com/connorgoggins/2d4d2ff6dca61494eb8151a5106fec6c

@ChaiBapchya
Copy link
Contributor

Incorrect way to rebase @connorgoggins

First get your master branch updated

git remote add upstream https://github.com/apache/incubator-mxnet
git checkout master
git fetch upstream master
git merge upstream/master
git push origin master

Once your fork master is sync'ed with remote master, rebase your branch on master

git checkout <branchname>
git rebase master
git push origin <branchname>

Let me know if this works!

@connorgoggins connorgoggins force-pushed the implement_nn_basic_ops branch 3 times, most recently from fbebf35 to f5279be Compare February 5, 2020 20:08
@connorgoggins
Copy link
Contributor Author

@mxnet-label-bot add [pr-awaiting-review]

@lanking520 lanking520 added the pr-awaiting-review PR is waiting for code review label Feb 5, 2020
@connorgoggins connorgoggins force-pushed the implement_nn_basic_ops branch 2 times, most recently from 9970fb7 to a5889ae Compare February 10, 2020 22:18
access2rohit
access2rohit approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@access2rohit access2rohit left a comment

Choose a reason for hiding this comment

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

lgtm!

@connorgoggins
Added paramters and state to ndarray
089ad8b
@connorgoggins
Added LRN
8641f9f
@connorgoggins
Added preloaded_multi_mp_sgd_mom_update
903c8dd
@connorgoggins
Added lamb_update_phase1
24859f4
@connorgoggins
Added lamb_update_phase2
e4e8f78
@connorgoggins
Dropped reversal
03b44ae
@connorgoggins
Finalized nn basic ops
4c3898f
@connorgoggins
Cleaned up code for linter
bf321cd
@connorgoggins
Refactored individual tests into generalized framework
f41bc8b
@connorgoggins
Refined logic, added default params
944b153
@connorgoggins
Fixed LRN param placement
5f4f639
@connorgoggins
Refactored default params for clarity
82b138a
@connorgoggins
Fixed lint errors
e9d7a38
@connorgoggins
Fixed BatchNorm issue
0e34266
@connorgoggins
Removed debugging comment
bf0d7d6
@connorgoggins
Added axis param for LayerNorm op
b74af9a
@connorgoggins
Fixed loc param issues
65b86a9
@connorgoggins
Linked Embedding backward issue in run_performance_test
ca9dcb3
@connorgoggins
Disabling problematic runs on gpu
82dd18e
@connorgoggins
Added myself to CONTRIBUTORS.md
87991c8
@connorgoggins
Addressed PR comments
72b857e
@connorgoggins
Cleaned up indentation
6d39346
@connorgoggins
Fixed DEFAULT_LABEL issue
8456520
@connorgoggins
Tightend up logic, established consistency with master
d836fc2
@connorgoggins
Fixed indent
e0dc303
apeforest
apeforest approved these changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution

@apeforest apeforest merged commit c2aff58 into apache:master Feb 19, 2020
zheyuye pushed a commit to zheyuye/incubator-mxnet that referenced this pull request Feb 19, 2020
@connorgoggins
Implement remaining nn_basic ops in opperf (apache#17456)
1a4fd20 
* Added SoftmaxOutput

* Added LinearRegressionOutput

* Added other regression ops

* Added SVMOutput

* Added L2, layer and instance norm

* gamma and beta to ndarray

* Reworked layer/instance norm

* Added Embedding

* Disabled backward on embedding

* Added Correlation

* Added data1 and 2 to ndarray

* Added SpatialTransformer

* Made loc ndarray type

* Run backward test

* Added IdentityAttachKLSparseReg

* Dropping grad

* Added sparseness target

* Added grad back

* Disabling backward for IdentityAttachKLSparseReg

* Trying to debug

* Print problematic op

* Another log

* Removing IdentityAttachKLSparseReg test for now

* Removed faulty test

* Added im2col

* Added col2im

* Added GroupNorm

* Added RNN

* Added paramters and state to ndarray

* Added LRN

* Added preloaded_multi_mp_sgd_mom_update

* Added lamb_update_phase1

* Added lamb_update_phase2

* Dropped reversal

* Finalized nn basic ops

* Cleaned up code for linter

* Refactored individual tests into generalized framework

* Refined logic, added default params

* Fixed LRN param placement

* Refactored default params for clarity

* Fixed lint errors

* Fixed BatchNorm issue

* Removed debugging comment

* Cleaned up indentation

* Added axis param for LayerNorm op

* Fixed loc param issues

* Linked Embedding backward issue in run_performance_test

* Disabling problematic runs on gpu

* Added myself to CONTRIBUTORS.md

* Addressed PR comments

* Fixed DEFAULT_LABEL issue

* Tightend up logic, established consistency with master

* Fixed indent
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request May 29, 2020
@connorgoggins @anirudh2290
Implement remaining nn_basic ops in opperf (apache#17456)
38e555f 
* Added SoftmaxOutput

* Added LinearRegressionOutput

* Added other regression ops

* Added SVMOutput

* Added L2, layer and instance norm

* gamma and beta to ndarray

* Reworked layer/instance norm

* Added Embedding

* Disabled backward on embedding

* Added Correlation

* Added data1 and 2 to ndarray

* Added SpatialTransformer

* Made loc ndarray type

* Run backward test

* Added IdentityAttachKLSparseReg

* Dropping grad

* Added sparseness target

* Added grad back

* Disabling backward for IdentityAttachKLSparseReg

* Trying to debug

* Print problematic op

* Another log

* Removing IdentityAttachKLSparseReg test for now

* Removed faulty test

* Added im2col

* Added col2im

* Added GroupNorm

* Added RNN

* Added paramters and state to ndarray

* Added LRN

* Added preloaded_multi_mp_sgd_mom_update

* Added lamb_update_phase1

* Added lamb_update_phase2

* Dropped reversal

* Finalized nn basic ops

* Cleaned up code for linter

* Refactored individual tests into generalized framework

* Refined logic, added default params

* Fixed LRN param placement

* Refactored default params for clarity

* Fixed lint errors

* Fixed BatchNorm issue

* Removed debugging comment

* Cleaned up indentation

* Added axis param for LayerNorm op

* Fixed loc param issues

* Linked Embedding backward issue in run_performance_test

* Disabling problematic runs on gpu

* Added myself to CONTRIBUTORS.md

* Addressed PR comments

* Fixed DEFAULT_LABEL issue

* Tightend up logic, established consistency with master

* Fixed indent
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ChaiBapchya ChaiBapchya ChaiBapchya approved these changes

@apeforest apeforest apeforest approved these changes

@access2rohit access2rohit access2rohit approved these changes

@aaronmarkham aaronmarkham Awaiting requested review from aaronmarkham

@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin

@marcoabreu marcoabreu Awaiting requested review from marcoabreu

@nswamy nswamy Awaiting requested review from nswamy

@szha szha Awaiting requested review from szha

Assignees
No one assigned
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@connorgoggins @ChaiBapchya @access2rohit @apeforest @lanking520