Skip to content

Conversation

@Krovatkin
Copy link
Contributor

@Krovatkin Krovatkin commented Aug 12, 2019

Addressing feedback we got from @SethHWeidman .
Mainly, making sure there's no confusion which serialized model should be loaded in a c++ app.

@Krovatkin
Copy link
Contributor Author

@brianjo would it be possible to merge a small update to this tutorial?

@SethHWeidman gave it a try and, unfortunately, it wasn't as clear as we would've hoped 😢

@netlify
Copy link

netlify bot commented Aug 12, 2019

Deploy preview for pytorch-tutorials-preview ready!

Built with commit 06343d7

https://deploy-preview-609--pytorch-tutorials-preview.netlify.com

@SethHWeidman
Copy link
Contributor

Appreciate you making these changes @Krovatkin.

Could I request one more change? In the line near the end of the tutorial:

root@4b5a67132e81:/example-app/build# ./example-app model.pt

Could we change this to:

root@4b5a67132e81:/example-app/build# ./example-app traced_resnet_model.pt

Part of my confusion in going through this tutorial was that the example-app we have only works with the ResNet model since we are passing in a 4D Tensor, so if someone tries to run this with custom_model.pt, they'll get a confusing error.

Adding a note to this effect - that we are pushing a 4D Tensor through our model here, so this example-app will only work with a CNN model - would be even better 🙂

@Krovatkin
Copy link
Contributor Author

Could we change this to:

Arrgh, I thought example.cpp was supposed to use MyModule. Fixed it.

@brianjo
Copy link
Contributor

brianjo commented Aug 12, 2019

I'll merge this after it rebases. Thanks!

@SethHWeidman
Copy link
Contributor

SethHWeidman commented Aug 12, 2019

Hey @Krovatkin, don't mean to be a pest about this, but I meant the very last time we call example-app, where it prints a slice of the first five elements of the result of feeding the Tensor through the model. It still says

root@4b5a67132e81:/example-app/build# ./example-app model.pt

Could we change that line to:

root@4b5a67132e81:/example-app/build# ./example-app <path_to_model>/traced_resnet_model.pt

Or Brian or I can change directly on master after this merges. Thanks!

Update: made the change. @brianjo I'm not 100% familiar with our Pull Request system, but making a change to a file on an existing PR like this shouldn't be an issue, right?

@brianjo brianjo merged commit 867acf2 into pytorch:master Aug 12, 2019
@Krovatkin
Copy link
Contributor Author

@SethHWeidman ah, you meant the output of a CMake script. Nice catch! Thanks for the update!

rodrigo-techera pushed a commit to Experience-Monks/tutorials that referenced this pull request Nov 29, 2021
fix confusion w.r.t. which model to serialize and use in cpp_export tutorial
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants