cppdocs icon indicating copy to clipboard operation
cppdocs copied to clipboard

Feedback

Open perone opened this issue 5 years ago • 4 comments

Hey, I just did a simple integration of libtorch as an addon for nodejs. The API worked very nice and I really enjoyed the design, it's very well done ! I'm adding in this issue some feedbacks (don't know if here is the best place, but here you go):

  • The torch::jit::load isn't exposed by just adding the torch/torch.h header, you have to include the torch/script.h as well, don't know if it is intentional but it took me some time to find the right header, maybe a documentation improvement could help;
  • There are some local paths hardcoded into the ATen cmake files, such as in share/cmake/ATen/ATenConfig.cmake: /Users/administrator/nightlies/2018_10_01/wheel_build_dirs/libtorch_2.7/pytorch/torch/lib/tmp_install/include;
  • Documentation is still missing some parts, especially regarding loading TorchScript, etc. Given that a lot of people will start to use the libtorch, I would be nice to have a expanded documentation on ATen/Torch API with some examples (I can contribute as well);
  • Problem when seeing the docs for the torch::jit::load (https://pytorch.org/cppdocs/api/function_namespacetorch_1_1jit_1ace2c44fb8af5905ae17834e81086b8a3.html#exhale-function-namespacetorch-1-1jit-1ace2c44fb8af5905ae17834e81086b8a3)
  • Many broken broken in the initial page of the docs (torch::nn, torch::optim, torch::data, torch::serialize, torch::jit and torch::python)

This is what I remember, if I see anything else I'll add it here as well.

perone avatar Oct 07 '18 03:10 perone

Hey, thanks a lot for the feedback @perone. I'll respond to everything in detail shortly.

goldsborough avatar Oct 08 '18 22:10 goldsborough

So, for the first point: This is sort of intended. The torch/torch.h header is for the C++ frontend, which is not strictly the same as the TorchScript C++ API. torch/script.h is intended to expose all necessary headers to interface with TorchScript, while torch/torch.h is intended to expose all necessary headers for defining and training models with the C++ frontend.

I will fix the ATenConfig.cmake thing. Did that cause an error for your, or are you just mentioning it? It looks wrong, but I haven't seen errors from this yet.

For documentation: Did you see the tutorial on exporting a model to C++ and loading it, https://pytorch.org/tutorials/advanced/cpp_export.html? Which tutorial would you liked to have had? I can write more.

I will fix the torch::jit::load issue. It also doesn't seem to have good docs anyway.

I fixed the various torch::* namespace links in https://github.com/pytorch/pytorch/pull/12521.

Let me know your thoughts and thanks so much for all the very helpful feedback!

goldsborough avatar Oct 10 '18 08:10 goldsborough

Hi @goldsborough thanks a lot for the quick reply ! Regarding the ATenConfig.cmake I haven't faced errors, just mentioned because I saw it by accident. I didn't saw the https://pytorch.org/tutorials/advanced/cpp_export.html tutorial, looks amazing. I guess the useful tutorials would be the ones like this one you did and focusing on the custom types (like the torch::jit::IValue that you mentioned for instance) and how to handle tensors, what are the copy/move semantics for them, etc. Also, I would avoid the auto on tutorials, it kind of omits the return type for who is reading, I understand it's very useful in code but for tutorials seems to hide important information.

Thanks again for the amazing work ! 👍

perone avatar Oct 12 '18 02:10 perone

I fixed the torch::jit::load docs. The text needs an update too, but at least the overloads show up correctly.

goldsborough avatar Oct 16 '18 13:10 goldsborough