-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[microNPU] Fix typo in depthwise to allow int8 weights #9299
Conversation
hi @lhutton1, thanks for fixing this. Is it possible to wrap this in a test case? |
Thanks @lhutton1 for fixing this, well spotted! As of a test case, I think all of our NPU tests exercise it since all of our weights in the tests are int8 (before that fix this check didn't actually check the weights dtype correctly) |
If there was a test case for this then the checks would've failed in |
The reason I ran into this was a use case where Depthwise had uint8 ifm/ofm but int8 weights. I'll look into adding this case in |
Hi @lhutton1, for making a test, you can also use |
Hi @lhutton1, I see you've updated the test cases, and apologies if I'm not seeing it, but they're all |
@Mousius see https://github.com/apache/tvm/pull/9299/files#diff-78a348fdab3d613a6644dc72ea3e697a8965c92a90117d345aa905c5027311f8R34-R35 :) The reason |
So that's the happy path, are there tests that check the error is fired. Could I remove this |
Hi @Mousius, yes, currently you can remove However, I'm not too sure I follow. I understand |
I would suggest that if we've added the check and we want to ensure its checking the right things we should have a test. |
In general I agree that the code behaviour should be tested, but in this case I agree with Luke. It is not a hugely consequential check and it is not enabled on all the builds, so it is quite hard to test is consistently. Luke is right that these checks are developer facing, IIRC all the other Relay operators also have checks like these that are not tested. |
Going back to the TVM Code Review Guidelines it can be seen in You can see similar testing added for other logging features here: #9350 |
I think testing this ICHECK is not trivial since the test would need to have different behaviours based on the build type, if it is a choice between spending time to come up with a test for it or removing it, I'd go for removing it. I'd be interested in what @mbaret and @manupa-arm think about it though |
Hi @ekalda, the current strategy appears to be testing with checks enabled as these tests have been introduced in other PRs; we just need a test which has a weight of neither int8 nor uint8 and to catch it using |
Hi @Mousius, I think you are actually right that we could test the ICHECK if we wanted to without too much trouble, so apologies for the confusion there. However, for what it's worth, I don't think it is a good idea since I don't think we should test everything just because we can test it. Tests are code to maintain, so we should think carefully what is worth testing and what isn't. There are currently 5184 ICHECKs in the codebase and I don't think many of them are tested. So if we were to test every developer facing assert, we would end up having to maintain lots of not particularly useful tests. |
Small typo means that ifm datatype is checked when it should be weight. Change-Id: I975e87c0bc91049ddee650f485f3a94841c55aba
Change-Id: I19fd934cda105187c86191313acf0fe88c75d244
Change-Id: Ibaf202287e2173bee638f9db8e553350ce1dff31
544b834
to
6ca9535
Compare
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.
Thanks for adding the test @lhutton1, I've just added one last point on the test itself.
However, I do wonder whether this implies every assertion added should also be tested?
All of the software teams I've worked in have tested for error cases, but we didn't add assertions in unreachable places as we had test coverage to ensure we never reached those lines. Happy to have a chat about this 😸
tests/python/contrib/test_ethosu/test_replace_depthwise_conv2d.py
Outdated
Show resolved
Hide resolved
…a more sensible location. Change-Id: I11f707ed4d81454765d7b3b597fa0c51e6909e3f
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.
@Mousius @ekalda @lhutton1 , I think this topic need a wider discussion on the forum, especially if we are deciding to enforce tests on ICHECKs (as opposed to CHECKs). I would suggest we continue that discussion a bit in the forum as there might be both pros and cons. cc : @tqchen @tkonolige |
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
@@ -126,7 +126,7 @@ bool EthosuDepthwiseConv2DRel(const Array<Type>& types, int num_inputs, const At | |||
ICHECK(ifm->dtype == DataType::UInt(8) || ifm->dtype == DataType::Int(8)) | |||
<< "Expected ethosu_depthwise_conv2d type(uint8) or type(int8) for ifm but was " | |||
<< ifm->dtype; | |||
ICHECK(weight->dtype == DataType::UInt(8) || ifm->dtype == DataType::Int(8)) | |||
ICHECK(weight->dtype == DataType::UInt(8) || weight->dtype == DataType::Int(8)) |
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.
Please use the diagnostic context to report these errors. Here is an example: https://github.com/apache/tvm/blob/main/src/relay/op/nn/nn.cc#L65-L68.
The diagnostic context is the correct way to report these errors. Here is an example: https://github.com/apache/tvm/blob/main/src/relay/op/nn/nn.cc#L65-L68. |
Convolution and depthwise convolution use the ICHECK format of error checking during type inference. This PR updates these checks to use the diagnostic context. This supersedes apache#9299. Change-Id: I67a06181440f84e9c38e02b9bf27218a6d6dd9d4
Thanks @tkonolige, we have rewritten these checks using the diagnostic context in #9470. Closing this PR as #9470 supersedes this. |
Small typo means that ifm datatype is checked when it should be weight.
cc @ekalda