[Hexagon] Do not use target test fixture in Hexagon tests

[mehrdadh]

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target 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

[Lunderberg]

All the individual changes look good, just a question on the design.

1. Should the tvm.target.hexagon function also set the host? That
   would remove the need for a separate testing-specific
   get_hexagon_target function.

2. 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.fixturedefhexagon_target(hexagon_cpu_version):
   returntvm.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']).

[Lunderberg]

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.

[mehrdadh]

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.

[Lunderberg]

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.

[Lunderberg]

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.

[mehrdadh]

yeah that would be great! I mentioned it to OSS team before.

[mehrdadh]

@Lunderberg thanks for the review!
Regarding your suggestions:

1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

[Lunderberg]

1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

2. That sounds reasonable, since we do have that difference for the host.

[mehrdadh]

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target 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

[Lunderberg]

All the individual changes look good, just a question on the design.

1. Should the tvm.target.hexagon function also set the host? That
   would remove the need for a separate testing-specific
   get_hexagon_target function.

2. 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.fixturedefhexagon_target(hexagon_cpu_version):
   returntvm.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']).

[Lunderberg]

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.

[mehrdadh]

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.

[Lunderberg]

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.

[Lunderberg]

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.

[mehrdadh]

yeah that would be great! I mentioned it to OSS team before.

[mehrdadh]

@Lunderberg thanks for the review!
Regarding your suggestions:

1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

[Lunderberg]

1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

2. That sounds reasonable, since we do have that difference for the host.

[mehrdadh]

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target 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

[Lunderberg]

All the individual changes look good, just a question on the design.

1. Should the tvm.target.hexagon function also set the host? That
   would remove the need for a separate testing-specific
   get_hexagon_target function.

2. 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.fixturedefhexagon_target(hexagon_cpu_version):
   returntvm.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']).

[Lunderberg]

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.

[mehrdadh]

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.

[Lunderberg]

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.

[Lunderberg]

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.

[mehrdadh]

yeah that would be great! I mentioned it to OSS team before.

[mehrdadh]

@Lunderberg thanks for the review!
Regarding your suggestions:

1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

[Lunderberg]

1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

2. That sounds reasonable, since we do have that difference for the host.

[mehrdadh]

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target 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

[Lunderberg]

All the individual changes look good, just a question on the design.

1. Should the tvm.target.hexagon function also set the host? That
   would remove the need for a separate testing-specific
   get_hexagon_target function.

2. 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.fixturedefhexagon_target(hexagon_cpu_version):
   returntvm.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']).

[Lunderberg]

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.

[mehrdadh]

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.

[Lunderberg]

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.

[Lunderberg]

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.

[mehrdadh]

yeah that would be great! I mentioned it to OSS team before.

[mehrdadh]

@Lunderberg thanks for the review!
Regarding your suggestions:

1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

[Lunderberg]

1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

2. That sounds reasonable, since we do have that difference for the host.

[mehrdadh]

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target 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

[Lunderberg]

All the individual changes look good, just a question on the design.

1. Should the tvm.target.hexagon function also set the host? That
   would remove the need for a separate testing-specific
   get_hexagon_target function.

2. 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.fixturedefhexagon_target(hexagon_cpu_version):
   returntvm.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']).

[Lunderberg]

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.

[mehrdadh]

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.

[Lunderberg]

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.

[Lunderberg]

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.

[mehrdadh]

yeah that would be great! I mentioned it to OSS team before.

[mehrdadh]

@Lunderberg thanks for the review!
Regarding your suggestions:

1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

[Lunderberg]

1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

2. That sounds reasonable, since we do have that difference for the host.

[mehrdadh]

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target 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

[Lunderberg]

All the individual changes look good, just a question on the design.

1. Should the tvm.target.hexagon function also set the host? That
   would remove the need for a separate testing-specific
   get_hexagon_target function.

2. 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.fixturedefhexagon_target(hexagon_cpu_version):
   returntvm.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']).

[Lunderberg]

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.

[mehrdadh]

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.

[Lunderberg]

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.

[Lunderberg]

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.

[mehrdadh]

yeah that would be great! I mentioned it to OSS team before.

[mehrdadh]

@Lunderberg thanks for the review!
Regarding your suggestions:

1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

[Lunderberg]

1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

2. That sounds reasonable, since we do have that difference for the host.

[mehrdadh]

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target 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

[Lunderberg]

All the individual changes look good, just a question on the design.

1. Should the tvm.target.hexagon function also set the host? That
   would remove the need for a separate testing-specific
   get_hexagon_target function.

2. 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.fixturedefhexagon_target(hexagon_cpu_version):
   returntvm.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']).

[Lunderberg]

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.

[mehrdadh]

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.

[Lunderberg]

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.

[Lunderberg]

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.

[mehrdadh]

yeah that would be great! I mentioned it to OSS team before.

[mehrdadh]

@Lunderberg thanks for the review!
Regarding your suggestions:

1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

[Lunderberg]

1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

2. That sounds reasonable, since we do have that difference for the host.

[mehrdadh]

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target 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

[Lunderberg]

All the individual changes look good, just a question on the design.

1. Should the tvm.target.hexagon function also set the host? That
   would remove the need for a separate testing-specific
   get_hexagon_target function.

2. 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.fixturedefhexagon_target(hexagon_cpu_version):
   returntvm.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']).

[Lunderberg]

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.

[mehrdadh]

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.

[Lunderberg]

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.

[Lunderberg]

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.

[mehrdadh]

yeah that would be great! I mentioned it to OSS team before.

[mehrdadh]

@Lunderberg thanks for the review!
Regarding your suggestions:

1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

[Lunderberg]

1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

2. That sounds reasonable, since we do have that difference for the host.