Skip to content
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

from_dlpack in spacy-transformers #741

Closed
svlandeg opened this issue Aug 10, 2022 · 10 comments
Closed

from_dlpack in spacy-transformers #741

svlandeg opened this issue Aug 10, 2022 · 10 comments

Comments

@svlandeg
Copy link
Member

svlandeg commented Aug 10, 2022

The test suite for spacy-transformers breaks in my local environment since PR #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?

@adrianeboyd
Copy link
Contributor

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.

@adrianeboyd
Copy link
Contributor

There is no particularly good way to specify this in the package dependencies.

@svlandeg
Copy link
Member Author

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...

@adrianeboyd
Copy link
Contributor

Daniël said that it's because the older version does not have __dlpack__ on arrays, so older numpy does not hit this code.

@polm
Copy link
Contributor

polm commented Aug 10, 2022

This also came up in the coref PR in spacy-experimental. It was pretty confusing but upgrading the Torch version did fix it.

@svlandeg
Copy link
Member Author

This is pretty bad though, because the error makes it look like there's an internal bug in Thinc/spaCy :(

@svlandeg
Copy link
Member Author

Maybe we should check the Torch version and raise a custom warning/error ourselves within Thinc?

@adrianeboyd
Copy link
Contributor

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?

@polm
Copy link
Contributor

polm commented Aug 15, 2022

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.

@svlandeg
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants