[oneDNN] Fix to issue #34554 #34623
[oneDNN] Fix to issue #34554 #34623
Conversation
|
@chenwhql Could you please approve PR-CI-APPROVAL. Its failure is a false positive due to fact that mechanism of checking PADDLE_ENFORCE_** works only on added lines of code. If we modify existing PADDLE_ENFORCE_** that spawn on multiple lines then CI mechanism will only see part of this call and pattern of VALID check may fail. |
|
@OliverLPH Could you please review it? |
paddle/fluid/platform/mkldnn_reuse.h
Outdated
| PADDLE_ENFORCE_NOT_NULL( | ||
| bwd_w_pd_, | ||
| platform::errors::Unavailable( | ||
| "Error: BWD_W_PD should be set when getting BWD grad of weights.")); |
There was a problem hiding this comment.
Error: here is needless, other similar cases are same
paddle/fluid/platform/mkldnn_reuse.h
Outdated
|
|
||
| std::shared_ptr<TBackward_params> AcquireBackwardWeightsPrimitive() { | ||
| PADDLE_ENFORCE_NOT_NULL(bwd_w_pd_, platform::errors::Unavailable( | ||
| "Error: BWD_PD should be set when " |
There was a problem hiding this comment.
I am not very sure.. if user see this error message, what changes they need to make? does the user understand BWD_PD?
There was a problem hiding this comment.
User of this API is a developer of oneDNN PaddlePaddle operator.This error should be understandable as it implies than some other calls of this API should be called before this one when implementing operator.
| mkldnn_engine, cpu_place) { | ||
| PADDLE_ENFORCE_EQ( | ||
| out_grad->dims(), in_x_grad->dims(), | ||
| platform::errors::InvalidArgument("The shape of softmax_grad's input " |
There was a problem hiding this comment.
we recommend that the expected value and actual value are also reported in the error message
paddle/fluid/platform/mkldnn_reuse.h
Outdated
| : platform::MKLDNNHandlerNoCachingT<T, dnnl::binary>(engine, cpu_place) { | ||
| PADDLE_ENFORCE_EQ( | ||
| x->layout(), DataLayout::kMKLDNN, | ||
| platform::errors::InvalidArgument("Wrong layout set for X tensor.")); |
There was a problem hiding this comment.
recommend give expected value and actual value
| // For Inplace src and and dst are the same memory object | ||
| // (jczaja) UT mechanics is testing inplace for this op | ||
| // regardless shapes, which is wrong when X is to be broadcasted as output | ||
| // is of bigger shape that X. |
There was a problem hiding this comment.
Could you clarify this comment, please?
There was a problem hiding this comment.
sure. Comment was updated.
|
@chenwhql Please continue your review |
|
@chenwhql Please approve PR-CI-APPROVAL PR-CI-APPROVAL is failing as I modified first and third line of PADDLE_ENFORCE_EQ (bold black): And problem is that script evaluating this change is only recieving modified lines . So it will not recieve : |
|
hi @jczaja , I compiled this PR and test #34554, there are still some models will crash. when ./build/model_test --single_thread=false --single_instance=false --test_groups="3"
could you please try re-test these models in your environment? |
|
@OliverLPH Hi, I will test given commandline and fix if there is any crash |
|
@jczaja this commit cause paddle compile failed, ci passed may be related by ccatch, so we revert this pr temporarily |
This reverts commit 0a5c99e.
|
@chenwhql Could you please share a full log of this build that failed? |
PR types
Bug fixes
PR changes
OPs
Describe
It is a fix to #34554 where models are using inplace execution. This inplace execution does not work well with caching mechanism so this PR is disabling caching for inplace capable ops: softmax, elementwise_*** and activation.