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

[Opperf] Filter out deprecated ops #15541

Merged
merged 10 commits into from
Aug 16, 2019
Merged

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Jul 15, 2019

Description

Filters out deprecated operators from _get_all_mxnet_operators() function.
To ensure they are not included while calculating MXNet operators

Current random_sampling_operators supports 16. Added 2 ops (normal, uniform)(although deprecated, they are currently supported. Hence)

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • 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

  • Removed uniform,normal op

Test/Verify

$ export PYTHONPATH=$PYTHONPATH:/path/to/incubator-mxnet/
$ python
>>> from benchmark.opperf.utils.op_registry_utils import _get_all_mxnet_operators
>>> for key in _get_all_mxnet_operators():
>>>     print(key)
>>>

### How to test/verify ###
Build
~~ ~~incubator-mxnet$ make -j8~~ ~~incubator-mxnet$ pip install -e python/.~~ ~~
Test
~~ ~~>>> import mxnet as mx~~ ~~>>> from mxnet import nd ~~ ~~>>> from benchmark.opperf.nd_operations.random_sampling_operators import run_mx_random_sampling_operators_benchmarks~~ ~~>>> print(run_mx_random_sampling_operators_benchmarks())~~ ~~

@ChaiBapchya
Copy link
Contributor Author

@mxnet-label-bot add [Benchmark, Operator]

@ChaiBapchya ChaiBapchya changed the title [Opperf] Add normal, uniform ops to random_sampling_operators [Opperf] Filter out deprecated ops Jul 16, 2019
@@ -55,6 +57,11 @@ def _select_ops(operator_names, filters=("_contrib", "_"), merge_op_forward_back
mx_operators = {}
operators_with_backward = []

# Filter out deprecated operators
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a "Deprecated" field in the operator metadata?
Because, listing operator by names is not scalable. Everytime we deprecate a operator, we need to come here to add it which soon because un-maintained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some digging around to find there's no exposed API from backend to retrieve deprecated ops. So at the moment, for the sake of this PR can we go ahead with that. I've created another issue to track the same. Sounds good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@piyushghai
Copy link
Contributor

@ChaiBapchya @sandeep-krishnamurthy What's the path forward for this PR ?

@ChaiBapchya
Copy link
Contributor Author

ChaiBapchya commented Aug 3, 2019

I'm guessing the right way for this PR would be
Create backend C API for getting deprecated ops (instead of my current method of hardcoding it). Just that I am bit stuck on other opperf that I have yet to write the backend part.

As discussed offline with @sandeep-krishnamurthy and @apeforest , this is temporary fix.
Permanent solution would require a design doc in itself. It would need changes on
operator registry, backend C API and at the same time needs to be backward compatible.

Given the current bandwidth, temporarily this fix sounds good to go.

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

@apeforest apeforest merged commit 5a4c01b into apache:master Aug 16, 2019
anirudhacharya pushed a commit to anirudhacharya/mxnet that referenced this pull request Aug 20, 2019
* normal, uniform ops

* Revert "normal, uniform ops"

This reverts commit f8d6f95.

* filter out deprecated ops

* Trigger notification

* additional deprecated ops

* Trigger notification

* Trigger notification

* Trigger notification
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants