thinc
thinc copied to clipboard
from_dlpack in spacy-transformers
The test suite for spacy-transformers breaks in my local environment since PR https://github.com/explosion/thinc/pull/686
numpy 1.23.1
spacy 3.4.1 (current master)
spacy-transformers 1.1.2 (current master)
thinc 8.1.0 (current master)
torch 1.9.0+cu111
e.g. when I run test_model_sequence_classification, it crashes at https://github.com/explosion/thinc/blob/master/thinc/util.py#L365:
> torch_tensor = torch.utils.dlpack.from_dlpack(xp_tensor)
E RuntimeError: from_dlpack received an invalid capsule. Note that DLTensor capsules can be consumed only once, so you might have already constructed a tensor from it once.
But, when I look at the code behind from_dlpack and run this directly (having confirmed my system doesn't hit the if statement checking the device):
dlpack = xp_tensor.__dlpack__()
torch_tensor = torch._C._from_dlpack(dlpack)
Then the test flies through.
Can anyone reproduce the failing tests on the spacy-transformers test suite with a relatively new numpy?
The problem is a combination of too-old-torch + too-new-numpy. numpy v1.23 works with torch v1.10+, but not torch v1.9.
There is no particularly good way to specify this in the package dependencies.
I'm not sure I understand why it's the combination that's causing this error? (and if it is, that's a pretty unclear error for users).
I'm mainly confused because the direct code path (the 2 alternative lines I cited) do in fact work...
Daniël said that it's because the older version does not have __dlpack__ on arrays, so older numpy does not hit this code.
This also came up in the coref PR in spacy-experimental. It was pretty confusing but upgrading the Torch version did fix it.
This is pretty bad though, because the error makes it look like there's an internal bug in Thinc/spaCy :(
Maybe we should check the Torch version and raise a custom warning/error ourselves within Thinc?
We haven't had any external reports of this so far, so my guess is that it's a pretty unusual version combination to have.
If we add something, only as a warning and only on import?
My initial reaction to this was to suspect that it was a bug in Thinc, but I also think this is not a version combination that normally happens in the wild. I think it's probably enough to have something that comes up in Google (like this thread) for the error message. Adding a warning at import would be fine too.
Ok, good points. Let's close this and leave as-is for now. If we do get external reports, we can consider adding a warning.