Repository navigation
[TFTRT] Rename ParameterizedOpConverterTest classes with more descriptive names - #45587
copybara-service[bot] merged 1 commit into
Conversation
bixia1
left a comment
bixia1
left a comment
There was a problem hiding this comment.
Looks good to me.
Just minor change to the PR subject and description because they will go to the commit message. Please don't forget the squash the commits into one as it will be ready for being approved.
(1) PR subject:
current: Renaming ParameterizedOpConverterTest classes to a more explicit name
suggested: Rename ParameterizedOpConverterTest classes with descriptive names
(2) suggested new PR description:
Rename the derived classes of ParameterizedOpConverterTestBase as follows for better readability:
OpConverterTest1 => OpConverterDynamicShape_FP32_Test
OpConverterTest2 => OpConverterDynamicShape_FP32_FP16_Test
OpConverterTest3 => OpConverterDynamicShape_FP32_FP16_INT8_Test
bixia1
left a comment
bixia1
left a comment
There was a problem hiding this comment.
When I think about this more, I think we should remove DynamicShape in the test name:
OpConverterTest1 => OpConverter_FP32_Test
OpConverterTest2 => OpConverter_FP32_FP16_Test
OpConverterTest3 => OpConverter_FP32_FP16_INT8_Test
Reason: the test template doesn't ensure that the test will test dynamic shape and the template can also support implicit batch test and explicit batch test. In particular ,for TRT5, we only instantiate implicit batch mode tests. For TRT7 we instantiate implicit batch mode tests, dynamic shape test and explicit batch mode tests.
|
Sounds fair with me. @tfeher what do you think ? If you agree, I'll push Bixia's recommendation |
tfeher
left a comment
tfeher
left a comment
There was a problem hiding this comment.
Thanks Jonathan for improving the naming conventions! I agree with Bixia's suggestion. There is one comment from my side, otherwise it looks good to me.
| class OpConverterTest3 : public ParameterizedOpConverterTestBase {}; | ||
| INSTANTIATE_TEST_CASE_P( | ||
| OpConvTestInstantiation3, OpConverterTest3, | ||
| OpConvTestInstantiation, OpConverterDynamicShape_FP32_FP16_INT8_Test, |
There was a problem hiding this comment.
This should have suffix _FP32_FP16_INT32 (see two lines below). This is meant for ops that can process shape (INT32) tensors.
INT8 mode we rarely test with the cpp unit tests. This is because we either need to set quantization ranges (as in #45599) or we would need calibration. Therefore INT8 testing is mostly done by the python integration test.
|
You missed the first comment in #45587 (review). I will try to make this change myself. |
bixia1
changed the title
bixia1
changed the title
google-ml-butler
Bot
added
kokoro:force-run
Rename the derived classes of
ParameterizedOpConverterTestBaseas follows for better readability:OpConverterTest1=>OpConverter_FP32_TestOpConverterTest2=>OpConverter_FP32_FP16_TestOpConverterTest3=>OpConverter_FP32_FP16_INT32_Test