refactor(framework): pytorch splitting refactor #2317
Conversation
|
Hello @larme, Thanks for updating this PR.
Comment last updated at 2022-02-28 11:25:55 UTC |
Codecov Report
@@ Coverage Diff @@
## main #2317 +/- ##
==========================================
+ Coverage 45.24% 48.81% +3.56%
==========================================
Files 106 134 +28
Lines 8281 8420 +139
==========================================
+ Hits 3747 4110 +363
+ Misses 4534 4310 -224
|
| @@ -43,8 +33,6 @@ | |||
| """ # noqa | |||
| ) | |||
There was a problem hiding this comment.
I don't think we need this block anymore since you already do an import check under .common.pytorch
There was a problem hiding this comment.
I'm thinking about just keeping these blocks in case one day we remove torch import from .common.pytorch. What's your opinion? @aarnphm
There was a problem hiding this comment.
Hmm why would we do so? I don't think we would ever do such things right?
There was a problem hiding this comment.
I made a change to import torch from .common.pytorch directly @aarnphm
|
You also have to add flags for |
|
the PyTorchContainer is failing the test, and for some reason the torchscript integration test is not running ? |
Let's fix this after |
Co-authored-by: Aaron Pham <[email protected]>
Co-authored-by: Aaron Pham <[email protected]>
Co-authored-by: Aaron Pham <[email protected]>
Co-authored-by: Aaron Pham <[email protected]>
Co-authored-by: Aaron Pham <[email protected]>
Co-authored-by: Aaron Pham <[email protected]>
Co-authored-by: Aaron Pham <[email protected]>
Co-authored-by: Aaron Pham <[email protected]>
larme
force-pushed
the
pytorch-refactor
branch
from
e0f5a52 to
6b153d5
Compare
| def _mapping(item: t.Union["ext.NpNDArray", torch.Tensor]) -> torch.Tensor: | ||
| if LazyType["ext.NpNDArray"]("numpy.ndarray").isinstance(item): | ||
| item = torch.Tensor(item, device=self._device_id) | ||
| return item |
There was a problem hiding this comment.
If it's already a tensor, we need to call to(device) on it as well, right?
There was a problem hiding this comment.
init with self._device_id so no need to call to(device)
There was a problem hiding this comment.
@aarnphm @bojiang I think you mean in self._configure() we have set default tensor storage type? But if the tensor is deserialized from DataContainer payload which is serialized in another machine and these 2 machine have mismatched device, then the tensor won't be converted to the proper device automatically. Also in my simple benchmark to(device) won't cost a lot if from_device and to_device is the same
Here's my simple demo:
https://gist.github.com/larme/7640f2d97d45c92946f1573b3eaaa0be
There was a problem hiding this comment.
I think calling to shouldn't affect perf that much. I think we just need to make sure that device is consistent across, so that #2230 won't happen.
There was a problem hiding this comment.
I think this change will make device more consistent?
There was a problem hiding this comment.
sounds good then, lets do that
|
paddlepaddle test will be addressed in a separate PR, seems like it just randomly fails all the time when using paddlehub module. |
* refactor(framework): split pytorch impl to pytorch and torchscript * style: reformat * style: minor fix * doc: fix pytorch/torchscript changes * chore: add torchscript github workflow * style: minor fix * Update bentoml/_internal/frameworks/common/pytorch.py Co-authored-by: Aaron Pham <[email protected]> * Update bentoml/_internal/frameworks/common/pytorch.py Co-authored-by: Aaron Pham <[email protected]> * Update bentoml/_internal/frameworks/common/pytorch.py Co-authored-by: Aaron Pham <[email protected]> * Update bentoml/_internal/frameworks/common/pytorch.py Co-authored-by: Aaron Pham <[email protected]> * Update bentoml/_internal/frameworks/pytorch.py Co-authored-by: Aaron Pham <[email protected]> * Update bentoml/_internal/frameworks/common/pytorch.py Co-authored-by: Aaron Pham <[email protected]> * Update bentoml/_internal/frameworks/common/pytorch.py Co-authored-by: Aaron Pham <[email protected]> * Update bentoml/_internal/frameworks/common/pytorch.py Co-authored-by: Aaron Pham <[email protected]> * style: minor fix * test: pytorch frameworks test save format * chore: update codecov.yml for torchscript * chore: github workflow add torchscript * test(framework): disable pytorch container batch_axis=1 test * chore: remove double import * fix: pytorch tensor to device Co-authored-by: Aaron Pham <[email protected]>
Description
Motivation and Context
How Has This Been Tested?
Checklist:
make formatandmake lintscript have passed(instructions).