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

Implement all miscellaneous ops #17511

Merged
merged 15 commits into from
Feb 14, 2020
Merged

Conversation

connorgoggins
Copy link
Contributor

@connorgoggins connorgoggins commented Feb 3, 2020

Description

This PR serves to implement all miscellaneous (not fitting into preexisting categories) operators in OpPerf. To achieve this, I created a file (benchmark/opperf/nd_operations/misc_operators.py) with a function (run_mx_misc_operators_benchmarks) to run all miscellaneous ops. Within this function, I used calls to run_performance_test for the misc ops with positional arguments and a call to a new function I wrote (get_remaining_miscellaneous_operators) in conjunction with run_op_benchmarks for the misc ops with keyword args. I also added a call to run_mx_misc_operators_benchmarks in opperf.py.

I also added functionality to disable backwards runs on ops that do not support them (SequenceLast) and registered a custom operator inline to test the Custom op.

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

  • A benchmark/opperf/nd_operations/misc_operators.py
  • M benchmark/opperf/opperf.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 p2.16xl w/ubuntu 16.04 and Mac OS with:

  1. Checkout branch and call function run_mx_misc_operators_benchmarks - runs all misc ops on relevant data
  2. Checkout branch and run opperf.py (full run of all ops)

Performance Results

Group of operator test - all misc ops (CPU)
Full OpPerf test (CPU)

Group of operator test - all misc ops (GPU)
Full OpPerf test (GPU)

@apeforest @access2rohit @ChaiBapchya

@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 3, 2020
@connorgoggins connorgoggins force-pushed the implement_misc_ops branch 2 times, most recently from f43c1e0 to d2d1510 Compare February 6, 2020 18:44
@ChaiBapchya
Copy link
Contributor

Looks like the alias mapping needs to get updated to search for the correct name in the MXNet profile.
For eg in case of Custom op, the profile will have CustomAddOne

@connorgoggins connorgoggins force-pushed the implement_misc_ops branch 2 times, most recently from 26f1ea3 to 3940595 Compare February 10, 2020 21:59
@connorgoggins
Copy link
Contributor Author

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.

"Custom": "SetValueOp" still looks incorrect. Will have dive deep to find what is the correct way of benchmarking CustomOp (since it is user defined so maybe there's no right/wrong answer.

But SetValueOp is an operator call that will happen for every op.

For eg even when we run profiler for add this is how the profile for operator part looks

Name                          Total Count        Time (ms)    Min Time (ms)    Max Time (ms)    Avg Time (ms)
    ----                          -----------        ---------    -------------    -------------    -------------
    DeleteVariable                        196           1.4490           0.0040           0.0250           0.0074
    _backward_broadcast_add               100         521.2320           4.8070           8.5970           5.2123
    SetValueOp                            100         645.8060           5.8820          10.0380           6.4581
    broadcast_add                         100         394.8910           3.5230           5.8790           3.9489

So here as well you can see DeleteVar and SetValueOp. So they can't be counted per se for Custom or any op.

Makes sense?

@connorgoggins
Copy link
Contributor Author

@ChaiBapchya makes sense. The reason I chose SetValueOp was because the only other option in the profiler output was DeleteVariable. Will dive deep on this to find the correct way of benchmarking Custom.

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.

LGTM

benchmark/opperf/utils/profiler_utils.py Show resolved Hide resolved
@ChaiBapchya
Copy link
Contributor

Again since we have updated this over and over, would be great to have the results updated (especially group of misc operators) Thanks

@connorgoggins
Copy link
Contributor Author

@ChaiBapchya all perf results are updated!

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!

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.

Thanks for your contribution!

@apeforest apeforest merged commit 8438d98 into apache:master Feb 14, 2020
zheyuye pushed a commit to zheyuye/incubator-mxnet that referenced this pull request Feb 19, 2020
* Initial commit - added first batch of misc ops

* Initial commit - added first batch of misc ops

* Added remaining misc ops, including Custom op logic

* Added more test cases, fixed lint errors

* Update documentation

* Added run_backward=True for ops supporting backwards runs

* Added issue link for bilinear UpSampling

* Added remaining misc ops, including Custom op logic

* Update documentation

* Updated alias map

* Fixed missing and incorrect alias issues

* Added remaining missing aliases

* Fixed Custom profile dump parsing and alias

* Switched to using sets for O(1) op membership checks

* Added fix for dtype issue in master
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request May 29, 2020
* Initial commit - added first batch of misc ops

* Initial commit - added first batch of misc ops

* Added remaining misc ops, including Custom op logic

* Added more test cases, fixed lint errors

* Update documentation

* Added run_backward=True for ops supporting backwards runs

* Added issue link for bilinear UpSampling

* Added remaining misc ops, including Custom op logic

* Update documentation

* Updated alias map

* Fixed missing and incorrect alias issues

* Added remaining missing aliases

* Fixed Custom profile dump parsing and alias

* Switched to using sets for O(1) op membership checks

* Added fix for dtype issue in master
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants