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
Srivastava kshitij new trt ops #332
Conversation
jaybdub
commented
Jun 9, 2020
- adds "enabled" flag to tensorrt_converter and add_module_test
- adds support for many TensorRT 7+ operations
- switches plugin build system to use PyTorch Extension
Tested with
|
@SrivastavaKshitij I've created this PR based off of your changes in #324 . It applies some refactoring
Are you able to test this for the configurations you use to make sure nothing is broken in the refactor? |
We should update Test Results
@jaybdub : I am not sure how to build for Pytorch < 1.3 without plugins. When u say Error related to Ngc container 19.07
|
Thanks for the fast response! Sorry, I forgot to push the By update README do you mean to add the test platform matrix? |
Ok . Let me test it quickly. Under Setup section in README, we should say use |
Getting the following error when running unit tests:
which makes sense because we didnt register plugin. I think we can have an |
Added disclaimer to README. Also, I set |
That's a very good idea ! We can also add test platform matrix. People use different combinations of Pytorch, torchvision and TRT. Test platform matrix will give them an idea as to which combinations have been tried and tested. We can add a dockerfile for reference Something like this:
This will help community in testing their environment easily |
Final Results
|
Thanks! This gives some confidence, I'll consider adding a test matrix. It might be useful to log warnings like you suggested. But we can probably save this for another smaller PR. Dockerfile would also be great. Jetson platforms are now heavily supporting cloud-native integration, so could be used there as well. I'd like to get this PR merged first, but then would be great to consider these other features. |
Thats perfect. I think this PR is ready to be merged :-) |
[DO NOT MERGE]: I ran the build again and I cant get plugins to build . I dont know what broke Now
Earlier:
|
Which system configuration is this? I only made a small change since last message, but shouldn't effect building. |
Ohhhh! It broke after |
Makes sense.
and |
There is no problem in the workflow @jaybdub : Sorry John, my bad. there was confusion on my side. I didnt add When i ran the following command, it works: So after |
Did you add Judging from the error you sent, it seems like it's attempting to build the plugin. My guess would be it's a linking / include directory issue in the extension. Right now it's hard coded to add the TensorRT paths for Jetson platforms. Maybe your docker had the environment set up so it didn't need this. Can you verify if this is the issue? If so, we can probably search for the correct path in setup.py, or if it can't be found allow user to pass path using flags. |
Ah, I see. So just to confirm -- building with |
@jaybdub : I ran build command again for all the combinations in test matrix and also ran unit tests. Everything is fine. Sorry for the false alarm ! |
No worries, good to hear! Going to do a few sanity tests on torchvision models and then merge. |
Srivastava kshitij new trt ops