-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[v1.4.x] mkldnn s8 conv API change for 1.4.x branch #13905
Conversation
@mxnet-label-bot add [pr-awaiting-review] |
@KellenSunderland could you help take a review this PR? The deadline of 1.4 is very near :( |
@TaoLv @xinyu-intel please help take a review too :) |
Please review and merge asap, this is the last PR blocking 1.4.0.rc1. Please keep an eye on builds and CI. |
@zheng-da @reminisce @eric-haibin-lin ping for review. |
Having a more detailed look now ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. Merging now to catch the code freezing of 1.4.0rc1. Please make sure the same changes will also happen on master branch, so we would not have API discrepancy between minor versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concern I have on this API design is the fact that types that are exposed in the public API come from mkldnn, which evolves independently from mxnet.
@szha , A better design is isolating public API with internal API, which is, create a new ndarray wrapper class that only exposes public API. But this probably can't be done within 1.4.x. Consider there're many mkldnn APIs already existing in ndarray.h, e.g. so I think it's fine for this time. |
Yes, IMO those shouldn't have happened either. We'd have to address those in 2.0. An alternative to extending the NDArray class is to pass around the metadata using nnvm within the abstraction of subgraph. |
Description
This is extracted from #13697, to make API change for 1.4.x branch.
@KellenSunderland @pengzhao-intel
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments