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

fix custom op for backward compatibility #8721

Closed
wants to merge 2 commits into from
Closed

fix custom op for backward compatibility #8721

wants to merge 2 commits into from

Conversation

ZiyueHuang
Copy link
Member

@ZiyueHuang ZiyueHuang commented Nov 20, 2017

Description

The default implementation infer_storage_type_backward in custom op has some problem.

Before this PR, the LR in sparse example is broken due to the custom op, and the added test will throw err message,

Error in mult.infer_type: Traceback (most recent call last):
  File "/home/hanfeng/zyh/mxnet/python/mxnet/operator.py", line 737, in infer_storage_type_backward_entry
    "stypes, got %d."%(total_outputs, len(ostype))
AssertionError: InferStorageTypeBackward Error: expecting 2 entries in returned output stypes, got 1.

[10:27:12] /home/hanfeng/zyh/mxnet/dmlc-core/include/dmlc/./logging.h:308: [10:27:12] src/operator/custom/custom.cc:383: Check failed: reinterpret_cast<CustomOpBackwardInferStorageTypeFunc>( params.info->callbacks[kCustomOpPropBackwardInferStorageType])( stypes.size(), stypes.data(), params.info->contexts[kCustomOpPropBackwardInferStorageType])

cc @eric-haibin-lin @anirudh2290

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • For user-facing API changes, API doc string has been updated. For new C++ functions in header files, their functionalities and arguments are well-documented.
  • To my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@eric-haibin-lin
Copy link
Member

@anirudh2290

@anirudh2290
Copy link
Member

anirudh2290 commented Nov 20, 2017

Thank you for fixing this and adding the tests! Yes, since the output stypes which is the in_grads would depend on arguments length and not output length. Can you please let me know what LR example you are referring to?

@@ -3652,6 +3652,42 @@ def create_operator(self, ctx, shapes, dtypes):
assert (x.grad.stype == 'csr')
assert (y.stype == 'csr')
assert (aux.stype == 'csr')

# test for backward compatibility, i.e. the correctness of default implementation of
Copy link
Member

Choose a reason for hiding this comment

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

Please remove whitespaces here and above.

@ZiyueHuang
Copy link
Member Author

@anirudh2290 Thanks for your comments. LR example is the linear_classification.py in example/sparse.

@mx.operator.register("mult")
class MultProp(mx.operator.CustomOpProp):
def __init__(self):
super(MultProp, self).__init__(need_top_grad=True)
Copy link
Member

Choose a reason for hiding this comment

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

Found an issue with custom_op when used with need_top_grad=False. Fixing here: #8725

@piiswrong
Copy link
Contributor

Closing this because we are reverting sparse support for custom op

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

Successfully merging this pull request may close these issues.

None yet

4 participants