Skip to content

[microNPU] Refactor codegen tests#9623

Merged
manupak merged 4 commits into
apache:mainfrom
mbaret:test-refactor
Dec 4, 2021
Merged

[microNPU] Refactor codegen tests#9623
manupak merged 4 commits into
apache:mainfrom
mbaret:test-refactor

Conversation

@mbaret

@mbaret mbaret commented Dec 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

The codegen tests had a lot of replicated functionality due to them being developed concurrently. This refactor consolidates those commonalities.

@mbaret

mbaret commented Dec 1, 2021

Copy link
Copy Markdown
Contributor Author

cc @ekalda @lhutton1 @NicolaLancellotti @dchauhan-arm - let me know what you think :)

@dchauhan-arm

dchauhan-arm commented Dec 1, 2021

Copy link
Copy Markdown
Contributor

broadly LGTM! Especially the change to making printing the command stream be optional so it doesn't print a novel in the logs. Just a few minor Q's inline

dchauhan-arm
dchauhan-arm reviewed Dec 1, 2021
View reviewed changes
Comment thread tests/python/contrib/test_ethosu/test_codegen.py Outdated
dchauhan-arm
dchauhan-arm reviewed Dec 1, 2021
View reviewed changes
Comment thread tests/python/contrib/test_ethosu/test_codegen.py Outdated
@manupak manupak changed the title Refactor Ethos-U codegen tests [microNPU] Refactor codegen tests Dec 1, 2021
ekalda
ekalda reviewed Dec 2, 2021
View reviewed changes

@ekalda ekalda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing the refactor, looks much better 😅 Other than resolving the test failures and rebasing on top of #9597, looks good!

ekalda
ekalda reviewed Dec 2, 2021
View reviewed changes
Comment thread tests/python/contrib/test_ethosu/test_codegen.py
Comment on lines +172 to +174
def _compare_ethosu_with_reference(
mod, input_data, output_data, accel_type, output_tolerance=0, print_cmm=False
):

@ekalda ekalda Dec 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if maybe this and the other helper functions could go to some more common location where they would be accessible to other files as well, so that we can make use of them in other tests that follow similar pattern, namely the tests in test_lookup_table.py

@lhutton1 lhutton1 Dec 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I believe this would also be useful for the tests in #9561 :)

@mbaret mbaret Dec 2, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I think we should move it to the 'end_to_end' infra when we add that directory.

NicolaLancellotti
NicolaLancellotti reviewed Dec 2, 2021
View reviewed changes

@NicolaLancellotti NicolaLancellotti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make these changes so that the tests use the same convention?

Comment thread tests/python/contrib/test_ethosu/test_codegen.py Outdated
Comment thread tests/python/contrib/test_ethosu/test_codegen.py Outdated
Comment thread tests/python/contrib/test_ethosu/test_codegen.py Outdated
Comment thread tests/python/contrib/test_ethosu/test_codegen.py Outdated
@mbaret
[microNPU] Refactor codegen tests
d3f7c3a 
Change-Id: I9c08520c9e03eb3fc32bd911b56c95981e851b4b
mbaret added 2 commits December 3, 2021 11:14
@mbaret
Fix params
6d3a80a 
Change-Id: I8cea69ed3824c3a0417bb67abbabce460c17c4c6
@mbaret
Remove prints
e1b44a5 
Change-Id: Iadf048e9590e724d73c2adac51bbe303de6f59a8
ekalda
ekalda approved these changes Dec 3, 2021
View reviewed changes

@ekalda ekalda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the refactor @mbaret

lhutton1
lhutton1 approved these changes Dec 3, 2021
View reviewed changes

@lhutton1 lhutton1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mbaret, a very welcomed refactor :) LGTM modulo others comments!

@mbaret
Address review comments
abc4aaa 
Change-Id: I56d647d86e3d495abe38b13cca349a71ec81cf4d
NicolaLancellotti
NicolaLancellotti approved these changes Dec 3, 2021
View reviewed changes

@NicolaLancellotti NicolaLancellotti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @mbaret

manupak
manupak approved these changes Dec 4, 2021
View reviewed changes

@manupak manupak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@manupak manupak merged commit 643d991 into apache:main Dec 4, 2021
@manupak

manupak commented Dec 4, 2021

Copy link
Copy Markdown
Contributor

Thanks @mbaret @ekalda @lhutton1 @NicolaLancellotti @dchauhan-arm !. This is merged now!

ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
@mbaret @ylc
[microNPU] Refactor codegen tests (apache#9623)
ef0f2f6 
* [microNPU] Refactor codegen tests

Change-Id: I9c08520c9e03eb3fc32bd911b56c95981e851b4b

* Fix params

Change-Id: I8cea69ed3824c3a0417bb67abbabce460c17c4c6

* Remove prints

Change-Id: Iadf048e9590e724d73c2adac51bbe303de6f59a8

* Address review comments

Change-Id: I56d647d86e3d495abe38b13cca349a71ec81cf4d
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
@mbaret @yangulei
[microNPU] Refactor codegen tests (apache#9623)
6c9ec29 
* [microNPU] Refactor codegen tests

Change-Id: I9c08520c9e03eb3fc32bd911b56c95981e851b4b

* Fix params

Change-Id: I8cea69ed3824c3a0417bb67abbabce460c17c4c6

* Remove prints

Change-Id: Iadf048e9590e724d73c2adac51bbe303de6f59a8

* Address review comments

Change-Id: I56d647d86e3d495abe38b13cca349a71ec81cf4d
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 12, 2022
@mbaret @yangulei
[microNPU] Refactor codegen tests (apache#9623)
6daac75 
* [microNPU] Refactor codegen tests

Change-Id: I9c08520c9e03eb3fc32bd911b56c95981e851b4b

* Fix params

Change-Id: I8cea69ed3824c3a0417bb67abbabce460c17c4c6

* Remove prints

Change-Id: Iadf048e9590e724d73c2adac51bbe303de6f59a8

* Address review comments

Change-Id: I56d647d86e3d495abe38b13cca349a71ec81cf4d
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@mbaret @ylc
[microNPU] Refactor codegen tests (apache#9623)
fdbd3d9 
* [microNPU] Refactor codegen tests

Change-Id: I9c08520c9e03eb3fc32bd911b56c95981e851b4b

* Fix params

Change-Id: I8cea69ed3824c3a0417bb67abbabce460c17c4c6

* Remove prints

Change-Id: Iadf048e9590e724d73c2adac51bbe303de6f59a8

* Address review comments

Change-Id: I56d647d86e3d495abe38b13cca349a71ec81cf4d
qsqqsqqsq-intellif pushed a commit to qsqqsqqsq-intellif/tvm that referenced this pull request Apr 29, 2022
@mbaret
[microNPU] Refactor codegen tests (apache#9623)
32123dc 
* [microNPU] Refactor codegen tests

Change-Id: I9c08520c9e03eb3fc32bd911b56c95981e851b4b

* Fix params

Change-Id: I8cea69ed3824c3a0417bb67abbabce460c17c4c6

* Remove prints

Change-Id: Iadf048e9590e724d73c2adac51bbe303de6f59a8

* Address review comments

Change-Id: I56d647d86e3d495abe38b13cca349a71ec81cf4d
@driazati driazati mentioned this pull request Jul 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NicolaLancellotti NicolaLancellotti NicolaLancellotti approved these changes
@ekalda ekalda ekalda approved these changes
@lhutton1 lhutton1 lhutton1 approved these changes
@manupak manupak manupak approved these changes
@areusch areusch Awaiting requested review from areusch
@comaniac comaniac Awaiting requested review from comaniac
@icemelon icemelon Awaiting requested review from icemelon
@jroesch jroesch Awaiting requested review from jroesch
@junrushao junrushao Awaiting requested review from junrushao
@merrymercy merrymercy Awaiting requested review from merrymercy
@tqchen tqchen Awaiting requested review from tqchen
@yzhliu yzhliu Awaiting requested review from yzhliu
+1 more reviewer
@dchauhan-arm dchauhan-arm dchauhan-arm left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mbaret @dchauhan-arm @manupak @NicolaLancellotti @ekalda @lhutton1