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

[OpPerf] Add Indexing ops #16253

Merged
merged 8 commits into from
Feb 5, 2020
Merged

[OpPerf] Add Indexing ops #16253

merged 8 commits into from
Feb 5, 2020

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Sep 23, 2019 •
edited
Loading

Description

Indexing Operators that are required as a part of extending OpPerf Operator coverage.

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 my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Added Indexing routines

@ChaiBapchya ChaiBapchya mentioned this pull request Oct 2, 2019
Open
@ChaiBapchya ChaiBapchya changed the title [WIP][OpPerf] Indexing op (Part 1 of GluonNLP op coverage) [OpPerf] Indexing op (Part 1 of GluonNLP op coverage) Jan 30, 2020
@ChaiBapchya ChaiBapchya changed the title [OpPerf] Indexing op (Part 1 of GluonNLP op coverage) [OpPerf] Add Indexing ops Jan 30, 2020
@ChaiBapchya
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 Jan 30, 2020
@ChaiBapchya ChaiBapchya mentioned this pull request Jan 30, 2020
Open
@ChaiBapchya
Copy link
Contributor Author

Results

  1. OpPerf by Category
    Command
from benchmark.opperf.nd_operations.indexing_routines import run_indexing_routines_benchmarks
run_indexing_routines_benchmarks()

Output

INFO:root:Begin Benchmark - gather_nd
INFO:root:Complete Benchmark - gather_nd
INFO:root:Begin Benchmark - one_hot
INFO:root:Complete Benchmark - one_hot
INFO:root:Begin Benchmark - ravel_multi_index
INFO:root:Complete Benchmark - ravel_multi_index
INFO:root:Begin Benchmark - slice
INFO:root:Complete Benchmark - slice
INFO:root:Begin Benchmark - slice_axis
INFO:root:Complete Benchmark - slice_axis
INFO:root:Begin Benchmark - slice_like
INFO:root:Complete Benchmark - slice_like
INFO:root:Begin Benchmark - take
INFO:root:Complete Benchmark - take
INFO:root:Begin Benchmark - where
INFO:root:Complete Benchmark - where

@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Jan 30, 2020 •
edited
Loading

CPU : https://gist.github.com/ChaiBapchya/1175946edf488d21156e4ac1d2218a5d

@ChaiBapchya
Copy link
Contributor Author

GPU : https://gist.github.com/ChaiBapchya/bc88f5ccb29aab8afef4563c56b5a11b

@ChaiBapchya ChaiBapchya mentioned this pull request Feb 2, 2020
Open
connorgoggins
connorgoggins approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@connorgoggins connorgoggins left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work :)

benchmark/opperf/nd_operations/indexing_routines.py Show resolved Hide resolved
benchmark/opperf/utils/benchmark_utils.py Show resolved Hide resolved
apeforest
apeforest reviewed Feb 4, 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.

Given that we are adding so many new op coverage thanks to your hard work, should we add regression tests to make sure the existing ops are not being negatively affected?

@ChaiBapchya
Copy link
Contributor Author

Sure..

But since we run

  1. entire opperf suite
  2. group/category specific suite

for every OpPerf PR and share the results as a gist
isn't that one kind of regression testing? Since it ensures we don't break previous ops in OpPerf.

But yes, I'm thinking of automating this utility + adding it to mxnet pip
Automating will make this broadly usable (consistently test/monitor ops)
Adding to pip (as revealed by talk with Tao) will make it accessible to end-users.

@apeforest what do you think?

access2rohit
access2rohit reviewed Feb 4, 2020
View reviewed changes
benchmark/opperf/utils/op_registry_utils.py
-------
{"operator_name": {"has_backward", "nd_op_handle", "params"}}
"""
# @ChaiBapchya unravel_index errors out on certain inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a TODO ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an open issue whose link I have already added.

access2rohit
access2rohit approved these changes Feb 4, 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! just one comment about TODO. If it is then use the following format

TODO(chaibapchya):

@ChaiBapchya
Copy link
Contributor Author

LGTM! just one comment about TODO. If it is then use the following format

TODO(chaibapchya):

@access2rohit Fair point. To prevent retriggering of CI, I will add this in the upcoming PRs. Thanks.

@ChaiBapchya ChaiBapchya mentioned this pull request Feb 4, 2020
Open
@apeforest apeforest merged commit dbf21e9 into apache:master Feb 5, 2020
@ChaiBapchya ChaiBapchya deleted the indexing_op branch February 5, 2020 18:48
zheyuye pushed a commit to zheyuye/incubator-mxnet that referenced this pull request Feb 19, 2020
@ChaiBapchya
[OpPerf] Add Indexing ops (apache#16253)
fdc991b 
* init

* updates

* fix lint in benchmark dir, fix pick documentation

* index ndarray fix

* add indexing ops to opperf, barring ones that fail

* add pick, fix lint issues, remove incorrect checks

* fix profiler doc
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request May 29, 2020
@ChaiBapchya @anirudh2290
[OpPerf] Add Indexing ops (apache#16253)
3151955 
* init

* updates

* fix lint in benchmark dir, fix pick documentation

* index ndarray fix

* add indexing ops to opperf, barring ones that fail

* add pick, fix lint issues, remove incorrect checks

* fix profiler doc
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 approved these changes

@connorgoggins connorgoggins connorgoggins approved these changes

@sandeep-krishnamurthy sandeep-krishnamurthy Awaiting requested review from sandeep-krishnamurthy

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.
5 participants
@ChaiBapchya @apeforest @access2rohit @connorgoggins @lanking520