-
Notifications
You must be signed in to change notification settings - Fork 743
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
[Pytorch] Add an adapter for c10::string_view #1392
Conversation
I mapped new Info("c10::basic_string_view<char>", "c10::string_view").annotations("@StringView").valueTypes("String", "BytePointer)) While for new Info("std::string").annotations("@StdString").valueTypes("BytePointer", "String").pointerTypes("BytePointer")) The difference in order of valueTypes results in functions returning Do you remember why it's this way for |
BytePointer is more general, yes.
|
Did a quick test with the original einsum example after building locally. Looking good now :) println(torch.einsum("ii", torch.randn(Seq(4, 4)))) // -2.5606 |
@sbrunk Please let us know if this sufficient for all your use cases! Thanks |
I have to admit I haven't done much testing with other functions using This is partly because it's much easier to test with the prebuilt CI snapshots. For instance, right now I'm traveling without access to my Linux build machine and I can't get the preset to build locally on macOS. So if you merge this now, I can't promise that I won't find any issue once I've improved test coverage. I don't want to cause you any additional overhead though, so if you'd rather like to wait until I've done more comprehensive testing, I'll do it as soon as I'm back/able to build locally again. |
Ok, in any case, the build fails on Windows, so that needs to be fixed before we can merge this:
|
The checks should pass now. It passed on linux because the parser is run, and not on windows. Is that on purpose ? |
See #1390
I didn't use the owner field of the adapter. I don't think there is a case where we want to free something when the adapter is deleted. Is there ? A string_view is supposed to be a not-owning view of some string.
@sbrunk, could you give that a try ?