Skip to content

Conversation

@u99127
Copy link

@u99127 u99127 commented Jul 6, 2021

This pull request cleans up the TOPI testuite for use on the AArch64
CI target by doing the following:

  • Introducing a script to run the tests on AArch64 with a suitable
    invocation of the llvm target string by setting the TVM_TEST_TARGETS
    environment variable.

  • Cleaning up the use of hard coded targets and moving the testsuite
    to testing more sensibly with the use of tvm.testing.enabled_targets.

  • Cleanup the use of tvm.target.create.

Putting this up for a test run on ci_gpu and ci_cpu to see the effects
of moving TOPI test runs to AArch64 CPU before firing up the Jenkins
changes.

The motivation was from #8361 to pipeclean and add this support.

Comment on lines 80 to 78
for device in ["llvm"]:
check_device(device)
for target, dev in tvm.testing.enabled_targets():
check_device(target, dev)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the schedules for sparse ops are not ready for GPU? I see the CI failed in cases like this.
We can add a TODO notes here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes agreed. there's a few of them like that which needs to be fixed. WIP.

Ramana Radhakrishnan added 4 commits August 3, 2021 21:52
This pull request cleans up the TOPI testuite for use on the AArch64
CI target by doing the following:

- Introducing a script to run the tests on AArch64 with a suitable
  invocation of the llvm target string by setting the TVM_TEST_TARGETS
  environment variable.

- Cleaning up the use of hard coded targets and moving the testsuite
  to testing more sensibly with the use of tvm.testing.enabled_targets.

- Cleanup the use of tvm.target.create.

- The above allows for the use of tests reasonably with the topi
  tests and cleans up what is needed from the testsuite.

Putting this up for a test run on ci_gpu and ci_cpu to see the effects
of moving TOPI test runs to AArch64 CPU before firing up the Jenkins
changes.

The motivation was from apache#8361 to pipeclean and add this support.
Change-Id: I70537a20bc683c490ef3a504a14136e7b0a1456d
Copy link
Contributor

@areusch areusch 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 working on this @u99127 !


for target, dev in tvm.testing.enabled_targets():
check_device(target, dev)
for device in ["llvm", "cuda"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

should these become @tvm.testing.requires_?

# for device in ['llvm -mcpu=skylake-avx512']:
for device in ["llvm"]:
check_device(device)
for target, dev in tvm.testing.enabled_targets():
Copy link
Contributor

Choose a reason for hiding this comment

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

same question--possible to use tvm.testing.parametrize_targets ?


for device in ["llvm"]:
check_device(device)
# Unable to use the following idiom and thus restrict to llvm only.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you comment why?


for device in ["llvm"]:
check_device(device)
# This test is not yet ready to use the idiom ,
Copy link
Contributor

Choose a reason for hiding this comment

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

could you say what's missing in the comment?

tvm.testing.assert_allclose(Y_tvm.numpy(), Y_np.astype("float32"), atol=1e-4, rtol=1e-4)

check_device("llvm")
# Unable to use the idiom below as the test isn't suitable for non llvm targets.
Copy link
Contributor

Choose a reason for hiding this comment

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

same question here

@masahi
Copy link
Member

masahi commented Jan 9, 2022

@u99127 Can you update or close this PR?

@jroesch jroesch added the needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it label Jan 19, 2022
@u99127 u99127 closed this Mar 24, 2022
@u99127
Copy link
Author

u99127 commented Mar 24, 2022

Abandoned in favour of other changes from @Mousius and others fixing this for AArch64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants