[microNPU] Refactor microNPU Codegen and Legalization tests#10250
[microNPU] Refactor microNPU Codegen and Legalization tests#10250dchauhan-arm wants to merge 6 commits intoapache:mainfrom
Conversation
dchauhan-arm
requested review from
areusch,
comaniac,
icemelon,
jroesch,
junrushao,
merrymercy,
tqchen and
yzhliu
as code owners
|
cc @manupa-arm @ekalda @mbaret @lhutton1 @NicolaLancellotti @jacobbohlin The original codegen and legalization test files still exist in this commit. When people are happy to merge, I'll push a commit that removes these files, and we can get it merged. |
dchauhan-arm
force-pushed
the
pers-refactor
branch
from
09ed916 to
e632818
Compare
Current legalization and codegen tests for microNPU operators all exist within the same two files that are worked on by multiple people within the team. For the purposes of future development, the two files are going to be restructured as folders containing individual test files for each operator. (End to end and legalization).
lhutton1
left a comment
lhutton1
left a comment
There was a problem hiding this comment.
Thanks @dchauhan-arm this makes the tests much easier to navigate. I did an initial pass, mostly just suggestions to remove code duplication further and naming suggestions.
One suggestion to improve consistency across the tests would be to name each file by just the operator i.e. convolution.py as we might add other tests to the file in the future that aren't for example tflite related. See what @manupa-arm and @ekalda think :)
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| """Test infrastructure for Arm(R) Ethos(TM)-U NPU related tests""" |
There was a problem hiding this comment.
Nit: update this comment to something like End to end tests for...
There was a problem hiding this comment.
ack! For the naming, agreed on tests simply being something along the lines of test_convolution.py as mentioned, waiting to hear from others on what they think
| import numpy as np | ||
| import tflite.Model |
There was a problem hiding this comment.
Nit: These imports can be removed
There was a problem hiding this comment.
A lot of imports to be cleaned up that I missed. ack -r all to these nits
| from tvm.relay.expr_functor import ExprMutator | ||
| from tvm.relay.op.annotation import compiler_begin, compiler_end | ||
| from tvm.relay.backend.contrib.ethosu import util | ||
| from tvm.relay.backend.contrib.ethosu import preprocess | ||
|
|
||
| from tvm.relay.op.contrib.ethosu import partition_for_ethosu | ||
| from tests.python.relay.aot.aot_test_utils import generate_ref_data | ||
|
|
||
| from tests.python.contrib.test_ethosu import infra |
There was a problem hiding this comment.
Nit: same as above, and elsewhere. It should be possible to detect imports that aren't used in your editor
| @pytest.mark.parametrize("accel_type", ACCEL_TYPES) | ||
| @pytest.mark.parametrize("ifm_shape", [(3, 2), (1, 15, 11, 7), (3, 1, 12), (400,)]) | ||
| @pytest.mark.parametrize("ifm_scale, ifm_zp, ofm_scale, ofm_zp", [(1, 0, 1, 0), (0.015, 3, 0.2, 5)]) | ||
| def test_ethosu_identity_codegen(ifm_shape, ifm_scale, ifm_zp, ofm_scale, ofm_zp, accel_type): |
There was a problem hiding this comment.
Nit: might be better to keep this name (and the filename) consistent with others by calling it test_ethosu_identity, and doing this elsewhere too
There was a problem hiding this comment.
ack! and agreed, I left test names unchanged, but the file names don't always follow the test name. Will go over them again while cleaning up other things to ensure parity.
| [(8, 4), (0,), False, False], | ||
| ], | ||
| ) | ||
| def test_mean(accel_type, ifm_shape, axis, keep_dims, use_same_quantization): |
There was a problem hiding this comment.
Might be better to split this into two separate tests and make use of the utility functions in infra to remove a lot of the boilerplate. One test could be called test_tflite_mean, while the other test_mean_to_avg_pool which generates the graph in relay for the legalization of mean to average pool - since this case is difficult to generate using the TFLite converter. WDYT?
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| """Test infrastructure for Arm(R) Ethos(TM)-U NPU related tests""" |
There was a problem hiding this comment.
Nit: check to something like Legalization tests for...
| mod = relay.transform.InferType()(mod) | ||
| mod = relay.transform.MergeComposite(pattern_table)(mod) | ||
| mod = relay.transform.AnnotateTarget("ethos-u")(mod) | ||
| mod = relay.transform.MergeCompilerRegions()(mod) | ||
| mod = relay.transform.InferType()(mod) | ||
| mod = relay.transform.PartitionGraph()(mod) | ||
| mod = relay.transform.InferType()(mod) | ||
| mod = preprocess.preprocess_ext_io()(mod) |
There was a problem hiding this comment.
Nit: A nice trick to avoid needing to run the infer pass multiple times is to use Sequential which will make sure pass dependencies are resolved. e.g.
seq = tvm.ir.Sequential([
relay.transform.MergeComposite(pattern_table),
relay.transform.AnnotateTarget("ethos-u"),
relay.transform.MergeCompilerRegions(),
relay.transform.PartitionGraph(),
preprocess.preprocess_ext_io()
])
mod = seq(mod)
Feel free to ignore though
| converter.inference_input_type = tf.int8 | ||
| converter.inference_output_type = tf.int8 | ||
| tflite_model = converter.convert() | ||
| return tflite_model |
There was a problem hiding this comment.
Could we remove a lot of this boilerplate similar to the end-to-end tests?
There was a problem hiding this comment.
For a lot of these, the similarities are there, over enough lines of code, but the dissimilarities don't always pertain to the same arguments if I wrote a boilerplate for them. It could be done, but all it would consistently remove is the generation of a representative dataset, and not every test uses the same np.random.rand unfortunately. Definitely room to take a second look at these however.
| @@ -0,0 +1,17 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
What's the difference between tests/python/contrib/test_ethosu/test_end_to_end and tests/python/contrib/test_ethosu/end_to_end? Looks like a renaming issue? :)
There was a problem hiding this comment.
Turns out git will quite happily rename the files within the folder, but leave an empty folder up too! Good spot, removing.
ekalda
left a comment
ekalda
left a comment
There was a problem hiding this comment.
Thanks for the change @dchauhan-arm, it's definitely an improvement to the sanity of our testing! I agree with Luke that we should name the files consistently, I'd also suggest some light grouping, e.g. all the various binary elementwise tests in the same file, then the files can be named by the operator, like test_<operator> (I think pytest wants them to start with test_ to find them).
| This module provides infrastructure to verify the correctness of | ||
| the command stream produced. | ||
| Currently it will invoke vela to generate a vela-optimized tflite | ||
| in which the command stream is contained as a custom operator. | ||
| This class include methods to parse the custom operator to extract | ||
| the command stream and perform an equivalency check for single operator | ||
| test cases. | ||
| """ |
There was a problem hiding this comment.
This is not true any more :) Maybe something along the lines "Utility functions to compare the output from the TFLite runtime to the NPU output"
areusch
added
needs-triage
4 participants
Current legalization and codegen tests for microNPU operators all exist
within the same two files that are worked on by multiple people within
the team. For the purposes of future development, the two files are
going to be restructured as folders containing individual test files for
each operator. (End to end and legalization).