Skip to content
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

Feedback #2

Open
perone opened this issue Oct 7, 2018 · 4 comments
Open

Feedback #2

perone opened this issue Oct 7, 2018 · 4 comments
Assignees

Comments

@perone
Copy link

perone commented Oct 7, 2018

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.

@goldsborough goldsborough self-assigned this Oct 8, 2018
@goldsborough
Copy link
Contributor

goldsborough commented Oct 8, 2018

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

@goldsborough
Copy link
Contributor

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 pytorch/pytorch#12521.

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

@perone
Copy link
Author

perone commented Oct 12, 2018

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 ! 👍

facebook-github-bot pushed a commit to pytorch/pytorch that referenced this issue Oct 13, 2018
Summary:
ezyang soumith

Partly addresses pytorch/cppdocs#2
Pull Request resolved: #12521

Differential Revision: D10374244

Pulled By: goldsborough

fbshipit-source-id: 8e9fe688cbaa2d2b0b96f721e5477ee8845b8f20
@goldsborough
Copy link
Contributor

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

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

No branches or pull requests

2 participants