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.

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

[UnitTests][Ethos-N] Mark unit tests as requiring Ethos-N #8873

Merged
merged 1 commit into from
Sep 4, 2021

Conversation

Lunderberg
Copy link
Contributor

  • Adds the decorator tvm.testing.requires_ethosn

  • Marks all tests in tests/python/contrib/test_ethosn as requiring ethosn instead of directly checking ethosn_available(). This way, they show up as skipped rather than passing.

Copy link
Contributor

@tkonolige tkonolige 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 this @Lunderberg!

Copy link
Member

@Mousius Mousius 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 this @Lunderberg, I was just curious whether we should have a tvm.testing.contrib ? Personally, I would rather have it as it is here, and it's an improvement either way 😸

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

In general LGTM. I remembered we have another test that can be marked with this new decorator, in tvmc.

See here: https://github.com/Lunderberg/tvm/blob/0b7319d6708df5b4ee7cf468a5214cb735107dfb/tests/python/driver/tvmc/test_compiler.py#L293-L297

@Lunderberg
Copy link
Contributor Author

@Mousius: Thanks for this @Lunderberg, I was just curious whether we should have a tvm.testing.contrib ? Personally, I would rather have it as it is here, and it's an improvement either way smile_cat

I think I'd lean toward having it here. If some of the tools in contrib require a lot of testing functionality, then I think it would make sense to break it out, but so far it's only been single functions here and there.

@leandron: In general LGTM. I remembered we have another test that can be marked with this new decorator, in tvmc.

Thank you for pointing that out, and I've updated it as well. A quick grep doesn't show any other folders that reference ethosn, so we should be set there.

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

LGTM, just need to solve the conflicts, and get it past CI.

- Adds the decorator `tvm.testing.requires_ethosn`

- Marks all tests in `tests/python/contrib/test_ethosn` as requiring
  ethosn instead of directly checking `ethosn_available()`.  This way,
  they show up as skipped rather than passing.

- Marks test_compile_tflite_module_with_external_codegen as requiring
  ethosn.
@Lunderberg
Copy link
Contributor Author

Sounds good, and merge conflicts are now resolved.

@masahi masahi merged commit 3a72d2f into apache:main Sep 4, 2021
@Lunderberg Lunderberg deleted the ethosn_mark_unittests branch September 4, 2021 12:26
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
- Adds the decorator `tvm.testing.requires_ethosn`

- Marks all tests in `tests/python/contrib/test_ethosn` as requiring
  ethosn instead of directly checking `ethosn_available()`.  This way,
  they show up as skipped rather than passing.

- Marks test_compile_tflite_module_with_external_codegen as requiring
  ethosn.
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
- Adds the decorator `tvm.testing.requires_ethosn`

- Marks all tests in `tests/python/contrib/test_ethosn` as requiring
  ethosn instead of directly checking `ethosn_available()`.  This way,
  they show up as skipped rather than passing.

- Marks test_compile_tflite_module_with_external_codegen as requiring
  ethosn.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants