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

[Onnx][UnitTests] Excluded additional onnx tests #8574

Closed
wants to merge 1 commit into from

Conversation

Lunderberg
Copy link
Contributor

  • The onnx tests test_basic_convinteger, test_convinteger_with_padding, test_range_float_type_positive_delta_expanded, and test_range_int32_type_positive_delta_expanded don't run correctly on CUDA targets, so they are added to the exclusion.

  • Parametrized over the relative directory name, rather than the full directory name. This improves readability of the pytest output, and keeps the same parametrized test name across different python version.

  • Changed the target-specific skips to check the target kind, rather than the full target string.

@Lunderberg
Copy link
Contributor Author

@mbrookhart Can you review this, especially the addition of the cuda-specific excluded tests?

Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

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

One confusion on parametarization, otherwise LGTM

pytest.skip()
break
@pytest.mark.parametrize("onnx_test", onnx_test_folders)
def test_onnx_nodes(target, dev, onnx_test):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing how target and dev get into the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a feature that I added in #8010 , where any unit test or fixture that accepts a target or dev argument is parametrized over all targets in tvm.testing.enabled_targets. Basically, the same as the explicit pytest.mark.parametrize that was there previously, or the tvm.testing.parametrize_targets, but without needing each function to reproduce the same boilerplate. The target parametrize is defined in tvm.testing._auto_parametrize_target, and the dev fixture based on it is in the conftest.py.

- The onnx tests `test_basic_convinteger`, `test_convinteger_with_padding`, `test_range_float_type_positive_delta_expanded`, and `test_range_int32_type_positive_delta_expanded` don't run correctly on CUDA targets, so they are added to the exclusion.

- Parametrized over the relative directory name, rather than the full directory name.  This improves readability of the pytest output, and keeps the same parametrized test name across different python version.

- Changed the target-specific skips to check the target kind, rather than the full target string.
@Lunderberg
Copy link
Contributor Author

Closing, this clean-up will be done along-side #8542, which was based on this.

@Lunderberg Lunderberg closed this Aug 3, 2021
@Lunderberg Lunderberg deleted the onnx_tests branch August 23, 2021 14:45
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.

2 participants