-
Notifications
You must be signed in to change notification settings - Fork 182
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
Load TorchScript modules #644
Conversation
return this; | ||
} | ||
|
||
#if false // These functions "work," but the native code doesn't seem to find any interesting information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to try to figure out why the scripts don't seem to contain any information on element type or dimensions.
src/TorchSharp/JIT/ScriptModule.cs
Outdated
{ | ||
if (!System.IO.File.Exists(filename)) | ||
throw new System.IO.FileNotFoundException(filename); | ||
return new ScriptModule(THSJIT_load(filename)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will blow up in some fashion. We're just interfacing with the libtorch support for TorchScript, so whatever it does. I'll add a negative unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments. LGTM otherwise.
Adding article on torchscript.
|
||
public unsafe override Tensor forward(Tensor tensor) | ||
{ | ||
using (var parray = new PinnedArray<IntPtr>()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tarekgh -- is there some way to avoid the new[]{} and use stackalloc here, instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to use PinnedArray at all too. You may try the following which will allow writing the whole method without any unsafe code and you'll not to do any managed allocations.
ReadOnlySpan<IntPtr> buffer = stackalloc IntPtr[1] { tensor.Handle };
var res = THSJIT_Module_forward(handle, (IntPtr)buffer, 1);
Added unit test for a script module formed from a function in Pytorch.
I assume this is related to this functionality: torch.jit.save For consistency, would it make sense to rename the extension *.dat in the unit tests to *.pt?
This will help separate 3 different file formats. *.pth and *.data [Torchsharp specific format for import and export] discussed in saveload.md With this new implementation, now *.pt is introduced to the saveload.md Instead of using model.ts in unit test, it seems the consensus is to keep it as model.pt (for torchscript file, load or save) |
The file extensions used in unit tests don't really matter. |
Added doc comments.
But, yes, there will now be three file formats:
|
I think *.ts is a good TorchSharp format for those involving importsd.py and exportsd.py |
Using the consensus practice used in Rust LibTorch wrapper, *.pt is meant for Torchscript, created in pytorch.jit.save, loaded in RustWrapper, save in RustWrapper. I believe they are all could be interchanged, I hope this is right. If so, *.pt could be a good format interchange between pytorch, Torchsharp and perhaps other LibTorch wrappers |
Another thought! .pt is used between pytorch and LibTorch c++ wrappers, including Torchsharp. Since .pt includes the architecture of networks and trained weights, .pt could be converted into Onnx using TorchSharp for interchange with ML.NET. Likewise, that interchange could be done in ML.NET. However, I think it makes more sense to have a utility to convert .pt to onnx in Torchsharp. |
That's what it's for. Please note that TorchSharp will lack the ability to create TorchScript files, even after this PR. We will just be able to load and save. |
Another thought! .pt is used between pytorch and LibTorch c++ wrappers, including Torchsharp. Since .pt includes the architecture of networks and trained weights, .pt could be converted into Onnx using TorchSharp for interchange with ML.NET. Likewise, that interchange could be done in ML.NET. However, I think it makes more sense to have a utility to convert .pt to onnx in Torchsharp. |
Agree, however, Torchsharp is able to load TorchSript files and save back TorchScript files though the libtorch c++. => that is loadable by pytorch or other libtorch wrappers |
I believe that's exactly how the ONNX exporter for Pytorch works. Since you have to start with Pytorch (not TorchSharp) in order to get a TorchScript file, I don't know why you would want to export to ONNX from TorchSharp. What is the scenario you have in mind? |
The exported ONNX help keep the .NET communities staying WITHIN .NET :-) Imagine! Many pytorch communities are sharing models using TorchScript formats. TorchSharp could serve as the starting point for importing these TorchShript format and allowing the .NET community to do further training or inference within Torchsharp OR exporting that to ONNX for inference using ML.NET |
By doing that, this addresses the need for Deep Learning support in ML.NET based on a survey done in April 2021. |
Not until you can start from scratch in TorchSharp. Right now, you cannot have a TorchScript file without starting in Python, where you can also create a ONNX file from the same model. Once we can do torch.jit.script() in TorchSharp, sure, but until then, this is still very limited functionality |
We do not have to start from scratch in TorchSharp :-) |
Likewise, many .NET users do not care where Onnx come from as long as they are available for use in ML.NET |
In order to write an ONNX exporter, we would need to go through the same work that we need to do in order to export to TorchScript. So, until we're in a position to do that, ONNX export will also have to wait. I agree that it's important, but it's separate from this functionality, which allows you to import TorchScript, modify it, and save it, but nothing else. One thing at a time. |
Agree ONE THING AT A TIME. The final goal of keeping DEEP machine learning within the .NET is happening! |
With this PR, TorchSharp supports loading modules that were created using torch.jit.{trace,script} with Pytorch.
No support for traced or scripted functions, yet. The PR also does not support tracing or scripting in TorchSharp.