Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[microNPU] Refactor codegen tests #9623

Merged
merged 4 commits into from
Dec 4, 2021
Merged

[microNPU] Refactor codegen tests #9623

merged 4 commits into from
Dec 4, 2021

Conversation

mbaret
Copy link
Contributor

@mbaret mbaret commented Dec 1, 2021 •
edited
Loading

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

@mbaret
Copy link
Contributor Author

mbaret commented Dec 1, 2021

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

@dchauhan-arm
Copy link
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

@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
Copy link
Contributor

@ekalda ekalda left a comment

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
tests/python/contrib/test_ethosu/test_codegen.py Outdated
Comment on lines 172 to 174
def _compare_ethosu_with_reference(
mod, input_data, output_data, accel_type, output_tolerance=0, print_cmm=False
):
Copy link
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

Copy link
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 :)

Copy link
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
Copy link
Contributor

@NicolaLancellotti NicolaLancellotti left a comment

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?

tests/python/contrib/test_ethosu/test_codegen.py Outdated Show resolved Hide resolved
tests/python/contrib/test_ethosu/test_codegen.py Outdated Show resolved Hide resolved
tests/python/contrib/test_ethosu/test_codegen.py Outdated Show resolved Hide resolved
tests/python/contrib/test_ethosu/test_codegen.py Outdated Show resolved Hide resolved
@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
Copy link
Contributor

@ekalda ekalda left a comment

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
Copy link
Contributor

@lhutton1 lhutton1 left a comment

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
Copy link
Contributor

@NicolaLancellotti NicolaLancellotti left a comment

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
Copy link
Contributor

@manupak manupak left a comment

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
Copy link
Contributor

manupak commented Dec 4, 2021

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

@dchauhan-arm dchauhan-arm dchauhan-arm left review comments

@manupak manupak manupak approved these changes

@lhutton1 lhutton1 lhutton1 approved these changes

@ekalda ekalda ekalda approved these changes

@NicolaLancellotti NicolaLancellotti NicolaLancellotti 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

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