-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Hexagon] Do not use target
test fixture in Hexagon tests
#12981
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the individual changes look good, just a question on the design.
-
Should the
tvm.target.hexagon
function also set the host? That
would remove the need for a separate testing-specific
get_hexagon_target
function. -
Should we have a separate
hexagon_target
fixture in
python/tvm/contrib/hexagon/pytest_plugin.py
, rather than a
utility function?hexagon_cpu_version = tvm.testing.parameter('v69') @tvm.testing.fixture def hexagon_target(hexagon_cpu_version): return tvm.target.hexagon(hexagon_cpu_version)
This would also make it easier to run tests with multiple different
cpu versions, or to restrict a test to a single version by using
@pytest.mark.parametrize('hexagon_cpu_version', ['v68'])
.
): | ||
"""conv2d-conv2d test""" | ||
# TODO: no support for padding in conv2d #2 | ||
pad2 = 0 | ||
|
||
target = "llvm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like none of the tests in this file actually exercise Hexagon, and only validate that the schedules used elsewhere have the expected behavior. Are they intended to exercise Hexagon hardware at some point? If not, we should keep the same target
fixture as non-Hexagon targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I reverted it. I did since I don't like the noise in the skip tests but we should fix that in TVM generally instead of hard coding the target here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I go back and forth on that part. I tend to prefer the explicitly reported and skipped tests, because otherwise a skipped test can appear as a successful test. But I also agree that having lists upon lists of skipped tests isn't useful for an interactive environment, and just adds noise.
tests/python/contrib/test_hexagon/conv2d/test_conv2d_blocked.py
Outdated
Show resolved
Hide resolved
) | ||
tune_tasks([task], **options) | ||
|
||
|
||
if __name__ == "__main__": | ||
sys.exit(pytest.main(sys.argv)) | ||
tvm.testing.main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side comment, thank you for updating these calls to use tvm.testing.main()
. I'd like to make a custom lint rule for it at some point, but there isn't one at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that would be great! I mentioned it to OSS team before.
@Lunderberg thanks for the review!
|
|
…2981) * remove target from test functions * refactor target_hexagon * refactor target * fix permission * cleanup * fix target * remove target fixture from test_2d_physical_buffers * fix target fixture in test_hexagon/conv2d tests * address comments
target
is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Addingtarget
in the test function would result in running those tests multiple times in the CI.This PR also refactors hexagon target in a function
cc @cconvey @csullivan @Lunderberg