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

Improve createOpState for custom ops #19103

Merged
merged 4 commits into from
Sep 11, 2020

Conversation

samskalicky
Copy link
Contributor

@samskalicky samskalicky commented Sep 9, 2020

Description

Support passing all args from createOpState to custom ops: context, input shapes & types

  • Also updates test_gemm.py and test_relu.py for custom ops on 2.0 now that Symbol.bind is deprecated

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

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

@mxnet-bot
Copy link

Hey @samskalicky , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [unix-gpu, miscellaneous, edge, sanity, windows-gpu, unix-cpu, centos-gpu, clang, centos-cpu, windows-cpu, website]


Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@samskalicky
Copy link
Contributor Author

@mseth10 @rondogency this is a small PR with just the change for custom stateful ops. Please review, thanks!

@samskalicky samskalicky changed the title [WIP] improve createOpState for custom ops Improve createOpState for custom ops Sep 10, 2020
@samskalicky samskalicky added the pr-awaiting-review PR is waiting for code review label Sep 10, 2020
@szha
Copy link
Member

szha commented Sep 10, 2020

thanks for the enhancement! as we are approaching the (beta) release of mxnet, do you think the API would be finalized with this PR?

@samskalicky
Copy link
Contributor Author

I think so, with this PR the examples are all up to date now with Gluon (having removed the symbol bind/executor stuff).

As with anything, as it gets more use we'll find ways to improve things. But we're ready to start helping users extend MXNet!

Copy link
Contributor

@mseth10 mseth10 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

@karan6181 karan6181 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank You for you contribution!

@rondogency
Copy link
Contributor

One quick request, in terms of the test, can you add an end-to-end test training example written in gluon like this one: #18167, so that we can have better test suite on performance to show to user

Copy link
Contributor

@rondogency rondogency left a comment

Choose a reason for hiding this comment

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

LGTM! the above comment can be addressed in the future PR

@samskalicky samskalicky merged commit e5a7814 into apache:master Sep 11, 2020
This was referenced Sep 11, 2020
samskalicky added a commit that referenced this pull request Sep 14, 2020
* initial commit

* incremented version number

Co-authored-by: Ubuntu <ubuntu@ip-172-31-6-220.us-west-2.compute.internal>
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