You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/10/17 00:34:17 UTC
[GitHub] [tvm] lhutton1 opened a new pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
lhutton1 opened a new pull request #9299:
URL: https://github.com/apache/tvm/pull/9299
Small typo means that ifm datatype is checked when it should be weight.
cc @ekalda
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-946831493
@Mousius see https://github.com/apache/tvm/pull/9299/files#diff-78a348fdab3d613a6644dc72ea3e697a8965c92a90117d345aa905c5027311f8R34-R35 :)
The reason `answer` at the end refers to `dtype` twice is because the input and output data types are being referenced, not the data type of the weights.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-953751034
Thanks for the discussion @Mousius @ekalda, I've a test for incompatible weight data types to get this patch in. However, I do wonder whether this implies every assertion added should also be tested?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 closed pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 closed pull request #9299:
URL: https://github.com/apache/tvm/pull/9299
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
ekalda commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-945531707
Hi @lhutton1, for making a test, you can also use `make_ethosu_conv2d` from `infra.py` , that will create the NPU op directly and you can intitialise it with "wrong" weight dtype. There are some examples of using it in `test_type_inference.py`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-950177530
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 edited a comment on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 edited a comment on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-953751034
Thanks for the discussion @Mousius @ekalda, I've added a test for incompatible weight data types to get this patch in. However, I do wonder whether this implies every assertion added should also be tested?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a change in pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
Mousius commented on a change in pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#discussion_r738329628
##########
File path: tests/python/contrib/test_ethosu/test_replace_depthwise_conv2d.py
##########
@@ -176,3 +244,29 @@ def _visit(stmt):
"NONE",
]
assert data[0] == answer, data[0]
+
+
+def test_incompatible_weight_data_type():
+ ifm = relay.var("ifm", shape=(1, 8, 8, 3), dtype="int8")
+
+ depthwise = make_ethosu_depthwise_conv2d(
+ ifm=ifm,
+ channels=3,
+ kernel_shape=(3, 2),
+ padding=(0, 0),
+ strides=(1, 1),
+ dilation=(1, 1),
+ activation="NONE",
+ ifm_layout="NHWC",
+ ofm_layout="NHWC",
+ weight_dtype="int16",
+ )
+
+ func = relay.Function(relay.analysis.free_vars(depthwise), depthwise)
+ mod = tvm.IRModule.from_expr(func)
+
+ with pytest.raises(TVMError) as err:
+ mod = relay.transform.InferType()(mod)
+
+ message = "Expected ethosu_depthwise_conv2d type(uint8) or type(int8) for weight but was int16"
+ assert message in str(err.value)
Review comment:
This can be flattened into:
```suggestion
message = "Expected ethosu_depthwise_conv2d type(uint8) or type(int8) for weight but was int16"
with pytest.raises(TVMError, match=message):
mod = relay.transform.InferType()(mod)
```
Similar to:
https://github.com/apache/tvm/blob/5ab22bbf25bed2034b757e4a0175bfd68e8773b6/tests/python/relay/test_name_transforms.py#L37-L38
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 edited a comment on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 edited a comment on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-948626646
Hi @Mousius, yes, currently you can remove `ICHECK` without affecting the tests.
However, I'm not too sure I follow. I understand `ICHECK` is an assertion. A test could be added to check that the assertion was fired for incorrect datatypes but I don't think that's good practice, since assertions should only exist as a programmer aid. ~~They're also only triggered in a debug build, so the test would fail if this is not the case~~ (I was incorrect about this point). Furthermore, interaction from a user perspective should mean this assertion is never encountered since we only pattern match and offload the operations (and compatible attributes) that are supported by the microNPU. Incompatible operations should never reach this level, and so the assertion shouldn't get triggered unless there is a programmer error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#discussion_r738725497
##########
File path: src/relay/op/contrib/ethosu/depthwise.cc
##########
@@ -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))
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-963145560
Thanks @tkonolige, we have rewritten these checks using the diagnostic context in #9470. Closing this PR as #9470 supersedes this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
ekalda commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-950958648
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
ekalda commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-951667725
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 edited a comment on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 edited a comment on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-948626646
Hi @Mousius, yes, currently you can remove `ICHECK` without affecting the tests, since we only test legal combinations of input datatypes at this level.
However, I'm not too sure I follow. I understand `ICHECK` is an assertion. A test could be added to check that the assertion was fired for incorrect datatypes but I don't think that's good practice, since assertions should only exist as a programmer aid. They're also only triggered in a debug build, so the test would fail if this is not the case. Furthermore, interaction from a user perspective should mean this assertion is never encountered since we only pattern match and offload the operations (and compatible attributes) that are supported by the microNPU. Incompatible operations should never reach this level, and so the assertion shouldn't get triggered unless there is a programmer error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 edited a comment on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 edited a comment on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-948626646
Hi @Mousius, yes, currently you can remove `ICHECK` without affecting the tests.
However, I'm not too sure I follow. I understand `ICHECK` is an assertion. A test could be added to check that the assertion was fired for incorrect datatypes but I don't think that's good practice, since assertions should only exist as a programmer aid. They're also only triggered in a debug build, so the test would fail if this is not the case. Furthermore, interaction from a user perspective should mean this assertion is never encountered since we only pattern match and offload the operations (and compatible attributes) that are supported by the microNPU. Incompatible operations should never reach this level, and so the assertion shouldn't get triggered unless there is a programmer error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
ekalda commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-950705074
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-954161422
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-944345195
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
ekalda commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-944389521
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-954142040
@Mousius @ekalda @lhutton1 ,
A relevant past discussion : https://discuss.tvm.apache.org/t/rfc-icheck-and-check-conventions-and-error-messages/8709
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-944459144
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 `test_replace_depthwise_conv2d.py`. Adding this case to other places will be more tricky since we rely on TFLite generated models as input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-946779533
Hi @lhutton1,
I see you've updated the test cases, and apologies if I'm not seeing it, but they're all `int8` or `uint8` so there's no test of the actual error case here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-950817210
Going back to the [TVM Code Review Guidelines](https://github.com/apache/tvm/blob/main/docs/contribute/code_review.rst#factors-to-consider-about-code-quality) it can be seen in `F2` that we aim to ensure code runs correctly in all possible settings. This PR was raised as the error was triggering in the incorrect setting, if we've considered this check as valuable to keep in the codebase and therefore want it to be robust to code changes and refactoring there should be associated testing or otherwise it should be removed.
You can see similar testing added for other logging features here: https://github.com/apache/tvm/pull/9350
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-951064198
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 `pytest.raises` - apologies if I've missed something here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-948082033
So that's the happy path, are there tests that check the error is fired. Could I remove this `ICHECK` without effecting any tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lhutton1 commented on pull request #9299: [microNPU] Fix typo in depthwise to allow int8 weights
Posted by GitBox <gi...@apache.org>.
lhutton1 commented on pull request #9299:
URL: https://github.com/apache/tvm/pull/9299#issuecomment-948626646
Hi @Mousius, yes, currently you can remove `ICHECK` without affecting the tests, since we only test legal combinations of input datatypes at this level.
However, I'm not too sure I follow. I understand `ICHECK` is an assertion. A test could be added to check that the assertion was fired but I don't think that's good practice, since assertions should only exist as a programmer aid. They're also only triggered in a debug build, so the test would fail if this is not the case. Furthermore, interaction from a user perspective should mean this assertion is never encountered since we only pattern match and offload the operations (and compatible attributes) that are supported by the microNPU. Incompatible operations should never reach this level, and so the assertion shouldn't get triggered unless there is a programmer error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org