Fix Bugs #3902
Fix Bugs #3902
Conversation
github-actions
bot
added
component: lowering
| } | ||
| } | ||
|
|
||
| DataType::DataType(c10::ScalarType t) { |
There was a problem hiding this comment.
These are type constructors not direct translators
| TORCHTRT_CHECK( | ||
| t == at::MemoryFormat::Contiguous || t == at::MemoryFormat::ChannelsLast, | ||
| "Tensor format is unsupported (" << t << ")"); | ||
|
|
There was a problem hiding this comment.
Same thing here, value is held by a TensorFormat object
There was a problem hiding this comment.
I misread this one. For the purpose of clarity, I think we should use m prefix for object member variables. Will revert these.
|
@leimao thanks for the PR. For the changes to the TS C++ API, these are not bugs. We define a set of wrapped enums that abstract the TRT and PyTorch versions. This makes it easier to write code that may mix the two APIs arbitrarily. Also as with FX, we are strongly pushing people to use Dynamo + AOTInductor to replace TorchScript as it is now deprecated in PyTorch and will be removed in PyTorch 2.14 |
@narendasan I believe there is still a bug, please review again. Thanks. |
| [](Stack& stack) { | ||
| auto attn_mask = pop(stack).to<at::Tensor>(); | ||
| if (attn_mask.scalar_type() == at::kBool) { | ||
| attn_mask = attn_mask; |
There was a problem hiding this comment.
This one is unnecessary and is very confusing.
| switch (t) { | ||
| case at::MemoryFormat::ChannelsLast: | ||
| value = TensorFormat::kChannelsLast; | ||
| break; |
There was a problem hiding this comment.
This one fixes an oversight of the implementation.
narendasan
left a comment
narendasan
left a comment
There was a problem hiding this comment.
LGTM, Would reiterate again that the torchscript frontend is in maintenance mode so unless there are bugs we likely will not change the implementation much. But we are highly interested in collecting reasons why Dynamo isnt working for your use case if thats the reason you are using TS so we can work to get those gaps closed prior to torchscript being removed
|
The test failures seems to be irrelevant to my change. Some warnings were treated as errors by compiler flags. |
I think I mentioned Dynamo worked in my case. But for some reason, in a different environment, I only have a lower version of Torch-TensorRT which does not seem to have the Dynamo path. Is there any performance difference between the FX path and Dynamo path based on your experience? Also could you please take a lot at the test failures? Thanks @narendasan. |
Yeah if you rebase it should resolve those issues
They should be mostly the same but we have not been actively optimizing the FX path since Dynamo has superseded FX |
I have a noob question. Please bear with me if I did not ask it correctly. Since Dynamo will also produce FX graph as intermediate representation, which should be the same or similar to the one captured by Torch FX, what's the advantage of Torch-TensorRT-Dynamo over Torch-TensorRT-FX? Thanks. |
Dynamo has better graph capture, and a fixed op set called Core ATen. This means dynamo has reproducible graphs and strong op coverage. Dynamo also fully supports current generation serialization workflows and other advanced use cases like dynamic shape. FX graphs can include basically any operator in PyTorch so its more likely to hit unsupported ops. Also there are more limitations in how the graphs are traced which makes it hard to use for production. |
2 participants
I believe there are some bugs due to typo in programming.