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

Add TorchScriptWrapper_v1 #802

Merged
merged 3 commits into from
Nov 24, 2022
Merged

Add TorchScriptWrapper_v1 #802

merged 3 commits into from
Nov 24, 2022

Conversation

danieldk
Copy link
Contributor

PyTorch Modules can be converted to TorchScript. TorchScript has the advantage that the model is serialized with the model parameters in a portable manner. Deserialization (in contrast to pickling) does not require certain Python types to be available. In fact, a TorchScript module can be loaded in C++ without a Python interpreter.

For Thinc and spaCy, supporting TorchScript has two main benefits:

  1. Since the model is serialized, we don't run into the issue that we have with the PyTorch wrapper where we have to construct the PyTorch model before deserializing its parameters but we can only know the model shapes through deserialization.

  2. When a model is rewritten for e.g. quantization, it is unwieldy to construct the rewritten model by hand. So, with the TorchScript wrapper we would first have to construct the original model and then reapply the graph transformation. This is quite bad for transformations that reduce the model size, since we end up temporarily allocating all parameters. This is not an issue with TorchScript, since we serialize the rewritten model.

PyTorch `Module`s can be converted to TorchScript. TorchScript has
the advantage that the model is serialized with the model parameters
in a portable manner. Deserialization (in contrast to pickling) does
not require certain Python types to be available. In fact, a TorchScript
module can be loaded in C++ without a Python interpreter.

For Thinc and spaCy, supporting TorchScript has two main benefits:

1. Since the model is serialized, we don't run into the issue that
   we have with the PyTorch wrapper where we have to construct the
   PyTorch model before deserializing its parameters but we can only
   know the model shapes through deserialization.

2. When a model is rewritten for e.g. quantization, it is unwieldy to
   construct the rewritten model by hand. So, with the TorchScript
   wrapper we would first have to construct the original model and
   then reapply the graph transformation. This is quite bad for
   transformations that reduce the model size, since we end up
   temporarily allocating all parameters. This is not an issue
   with TorchScript, since we serialize the rewritten model.
@svlandeg
Copy link
Member

@danieldk : can you have a look at the conflicts?

@danieldk
Copy link
Contributor Author

@danieldk : can you have a look at the conflicts?

Fixed.

@svlandeg svlandeg added enhancement Feature requests and improvements interop / pytorch PyTorch interoperability serialization Saving and loading models labels Nov 22, 2022
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be great to have! I'm particularly enthousiastic about the prospect of resolving the initialization/deserialization shape issues 🎉

thinc/layers/torchscriptwrapper.py Outdated Show resolved Hide resolved
thinc/layers/torchscriptwrapper.py Show resolved Hide resolved
thinc/layers/torchscriptwrapper.py Outdated Show resolved Hide resolved
thinc/layers/torchscriptwrapper.py Outdated Show resolved Hide resolved
thinc/shims/__init__.py Outdated Show resolved Hide resolved
thinc/shims/torchscript.py Show resolved Hide resolved
thinc/shims/torchscript.py Outdated Show resolved Hide resolved
website/docs/api-layers.md Outdated Show resolved Hide resolved
website/docs/usage-frameworks.md Outdated Show resolved Hide resolved
@danieldk danieldk marked this pull request as draft November 23, 2022 11:23
@danieldk
Copy link
Contributor Author

I put the PR in draft for the moment, since I am still wondering if it's best to pass None as defaults to the conversion functions.

@svlandeg
Copy link
Member

I put the PR in draft for the moment, since I am still wondering if it's best to pass None as defaults to the conversion functions.

As discussed, I'd keep it as is.

@danieldk danieldk marked this pull request as ready for review November 23, 2022 12:50
@svlandeg svlandeg merged commit 9daaae5 into master Nov 24, 2022
@svlandeg svlandeg deleted the feature/torchscriptwrapper branch November 24, 2022 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements interop / pytorch PyTorch interoperability serialization Saving and loading models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants