-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Re-enable static cached_op optimization #14931
Conversation
Was there a test added in the process of resolving the linked issue? |
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.
Would you mind elaborating what caused the issue and how you fixed it?
@marcoabreu This is a performance issue in the gluonNLP so any suggestions for adding performance test on CI? |
@@ -705,8 +705,10 @@ void CachedOp::StaticRunOps( | |||
arg_shapes.emplace_back(ndinput->shape()); | |||
arg_dtypes.emplace_back(ndinput->dtype()); | |||
} | |||
state.op_states[i] = createop[node.source->op()]( | |||
node.source->attrs, default_ctx, arg_shapes, arg_dtypes); | |||
if (!config_.static_shape) { |
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.
@eric-haibin-lin This is the only changed line to fix this PR. We should re-create op state if static_shape
is false.
@mxnet-label-bot add [pr-awaiting-review] |
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
@eric-haibin-lin @marcoabreu please help take a review :) |
Will merge this PR at the end of today if there is no further comment. |
Description
This PR firstly filed at #14785, but reverted because of dmlc/gluon-nlp#690. Now it's fixed.
The fixed output is,
@pengzhao-intel @eric-haibin-lin
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments