Refactor onnx conversion #100
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D21072663 |
marksibrahim
added a commit
to marksibrahim/CrypTen
that referenced
this pull request
Apr 16, 2020
Summary: Pull Request resolved: facebookresearch#100 This diff refactors CrypTen's conversion of onnx models to allow for future alterations of the imported onnx graph. Changes include * moving logic of hairy onnx conversions into a separate module (`onnx_converter.py`) with its own tests * adding several tests for helper functions to map onnx graphs to crypten modules * refactoring more complex functions such as `from_pytorch` and `from_onnx` using helpers for easier testings / debugging * using a context manager for all `io.BytesIO()` streams to ensure resource is released Differential Revision: D21072663 fbshipit-source-id: 03b4957d481a98b986e89859212e2c5a59bcf891
marksibrahim
force-pushed
the
export-D21072663
branch
from
e90237a to
de27bbc
Compare
|
This pull request was exported from Phabricator. Differential Revision: D21072663 |
youben11
reviewed
Apr 19, 2020
crypten/nn/onnx_converter.py
Outdated
Comment on lines
50
to
59
| with io.BytesIO() as f: | ||
| _export_pytorch_model(f, pytorch_model, dummy_input) | ||
|
|
||
| # update ONNX symbolic registry with CrypTen-specific functions | ||
| _update_onnx_symbolic_registry() | ||
|
|
||
| # export again so the graph is created with CrypTen-specific registry | ||
| with io.BytesIO() as f: | ||
| f = _export_pytorch_model(f, pytorch_model, dummy_input) | ||
| f.seek(0) |
There was a problem hiding this comment.
It would be great if the logic here is put into a separate function. An interesting use-case for PySyft is where we can serialize a pytorch model into this bytes object, send it through the network, then build the crypten model in another machine.
There was a problem hiding this comment.
Great, happy to restructure. I'll update the pull request.
Summary: Pull Request resolved: facebookresearch#100 This diff refactors CrypTen's conversion of onnx models to allow for future alterations of the imported onnx graph. Changes include * moving logic of hairy onnx conversions into a separate module (`onnx_converter.py`) with its own tests * adding several tests for helper functions to map onnx graphs to crypten modules * refactoring more complex functions such as `from_pytorch` and `from_onnx` using helpers for easier testings / debugging * using a context manager for all `io.BytesIO()` streams to ensure resource is released Differential Revision: D21072663 fbshipit-source-id: ad8930d470b018f0f2733a0a25705c38affcdfda
marksibrahim
force-pushed
the
export-D21072663
branch
from
de27bbc to
473a18c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D21072663 |
youben11
reviewed
Apr 21, 2020
Comment on lines
+58
to
+72
| def _from_pytorch_to_bytes(pytorch_model, dummy_input): | ||
| """Returns I/O stream containing onnx graph with crypten specific ops""" | ||
| # TODO: Currently export twice because the torch-to-ONNX symbolic registry | ||
| # only gets created on the first call. | ||
| with io.BytesIO() as f: | ||
| _export_pytorch_model(f, pytorch_model, dummy_input) | ||
|
|
||
| # update ONNX symbolic registry with CrypTen-specific functions | ||
| _update_onnx_symbolic_registry() | ||
|
|
||
| # export again so the graph is created with CrypTen-specific registry | ||
| f = io.BytesIO() | ||
| f = _export_pytorch_model(f, pytorch_model, dummy_input) | ||
| f.seek(0) | ||
| return f |
|
This pull request has been merged in 1aa8ced. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This diff refactors CrypTen's conversion of onnx models to allow for future alterations of the imported onnx graph. Changes include
onnx_converter.py) with its own testsfrom_pytorchandfrom_onnxusing helpers for easier testings / debuggingio.BytesIO()streams to ensure resource is releasedDifferential Revision: D21072663