dlpack icon indicating copy to clipboard operation
dlpack copied to clipboard

Add tests for numpy, tensorflow, torch, and mxnet

Open tirthasheshpatel opened this issue 2 years ago • 8 comments

This PR adds basic tests for some python packages that support DLPack. Currently, it only tests CPU arrays. Some observations

  • Tensorflow doesn't provide a from_dlpack function and both Tensorflow and MXNet don't have the __dlpack__, __dlpack_version__ dunder methods. These need to be updated.
  • Tensorflow fails to import some NumPy arrays (most probably a bug in the deletion mechanism). These tests have been skipped for now.

More tests can be added in follow-up PRs. These tests are triggered for each commit on main/pr. Should we instead schedule them to run every week or month? GPU isn't available so I am not sure if we can test libraries like CuPy on the CI. Is is possible to emulate GPU or other devices in the CI?

tirthasheshpatel avatar May 02 '22 07:05 tirthasheshpatel

It would be nice to get some eyes on this. Once there are tests for the current behaviour using different libraries, it should be easier to judge the impact of the changes in #104

mattip avatar May 09 '22 14:05 mattip

I am wondering about this: If we include this test set in the array API tests (@asmeurer), maybe each library can run it in their own CI so we don't have to worry about GPU access? (cc: @kmaehashi)

leofang avatar May 09 '22 14:05 leofang

I'm not sure how the DLPack test suite should work in relation to the array API tests test suite. My initial thought is that it makes sense for DLPack to have its own test suite, since DLPack is independent of the array API. I believe we currently don't have any DLPack tests in the array API test suite beyond just basic testing if the methods exist and run without erroring. If DLPack itself is tested extensively by its own test suite, then perhaps the array API test suite can just defer to that, or pull it in somehow.

CC @honno @rgommers @kgryte

asmeurer avatar May 09 '22 20:05 asmeurer

I am thinking the tests from this PR (and future PRs) should have been added to each library's test suite when they claim they support DLPack. Without a universal test in this repo that can target every library I worry that we're just reinventing the wheel here... But, there's no way to write a universal test without an agreed-upon interface, and the array API could serve as a good proxy.

leofang avatar May 09 '22 20:05 leofang

To add onto @leofang's comment, for some given library (e.g. NumPy), it is not easy to test the DLPack support against other tensor libraries (e.g. PyTorch) since it would add extra dependencies for the project and affect the CI time.

tirthasheshpatel avatar May 09 '22 21:05 tirthasheshpatel

Sorry I didn't read Tirth's code until now. This PR essentially proposes to do an N-to-N conversion test across N libraries. The test suite would be hard to maintain unless 1. we use the array API to be backend-agnostic, or 2. we have an automation to auto-generate the added test scripts.

leofang avatar May 09 '22 22:05 leofang

Perhaps in the future, if N becomes large, it might be hard to maintain the tests. For now, with N = 4 there are ~16 tests. Each library has a different pattern for using DLPack, so it is not easy to see how to automate it today. It would be good to put this in now, and automate it when N grows larger and the use of DLPack becomes more standardized.

mattip avatar May 10 '22 03:05 mattip

@tqchen Any thoughts on this?

tirthasheshpatel avatar May 16 '22 06:05 tirthasheshpatel