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

Using torchvision C++ API without installing python #2692

Closed
stu1130 opened this issue Sep 20, 2020 · 16 comments · Fixed by #5190
Closed

Using torchvision C++ API without installing python #2692

stu1130 opened this issue Sep 20, 2020 · 16 comments · Fixed by #5190

Comments

@stu1130
Copy link

stu1130 commented Sep 20, 2020

❓ Questions and Help

I have followed the document and built the libtorchvsion.dylib but when I ran otool -L libtorchvsion.dylib it showed

libtorchvision.dylib:
	@rpath/libtorchvision.dylib (compatibility version 0.0.0, current version 0.0.0)
	@rpath/libc10.dylib (compatibility version 0.0.0, current version 0.0.0)
	@rpath/libpython3.8.dylib (compatibility version 3.8.0, current version 3.8.0)
	@rpath/libtorch.dylib (compatibility version 0.0.0, current version 0.0.0)
	@rpath/libtorch_cpu.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 902.1.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.100.1)

Is there a way to get rid of libpython3.8.dylib, I think it is required for using python custom operators and we will only use C++.

@lanking520
Copy link

lanking520 commented Sep 20, 2020

I managed to build the so with the hack:

https://github.com/pytorch/vision/blob/master/torchvision/csrc/cuda/vision_cuda.h#L2 change this line to include torch/torch.h.

Same with CPU. Also disable the unncessary pybind API call.

  • need change cmake to disable all python3 packages.

However I don't feel like this is the right way to do. Is there any side effect if I disabled all python access?

@fmassa
Copy link
Member

fmassa commented Sep 21, 2020

@mthrok has been looking into something like that for torchaudio and torchtext, maybe his findings could apply for torchvision as well? We might need to move away from cpp_extensions for building the C++ extensions, and instead use CMake for that.

@mthrok
Copy link
Contributor

mthrok commented Sep 24, 2020

I am working on gathering requirements and specing out cmake build for torchtext and torchaudio, which includes both Python extension module and C++ library. I thought torchvision has already have cmake build so did not look into it but if it's something beneficial to torchvision, I can include that one too.

So as to build C++ library, the source code cannot use things that are related to Python. This includes PyBind11 and <torch/extension.h>, which, in my understanding, is a wrapper of PyBind11 (I could be wrong here). So it will be easier if we can get rid of <torch/extension.h> and replace it with <torch/script.h>, but this will involve some code change on Python side too because the way C++ functions are exposed are different. That is, with PyBind11, the extension module is treated as a regular Python module, but TorchScript based binding put all the functions under torch.ops or torch.classes.

The way I am thinking of cmake build is some thing like the following;

  • CMakeLists.txt exposes two targets; C++ library and Python extension.
  • These two targets depend on internal targets which include all the third-party libraries and custom C++ objects that does not include any Python-related code.
  • When building the Python extension, setup.py resorts to call cmake command with supplement arguments, like Python header/library locations and PyTorch specific configurations.
  • With this CMakeLists.txt, one can just add domain library as third party and do add_subdirectory(vision/text/audio).

Here is an example how Python extension can be build with cmake via setup.py.

@fmassa
Copy link
Member

fmassa commented Sep 24, 2020

We already solved the torch.ops part in torchvision, and we also depend only on <torch/script.h>

#include <torch/script.h>
, but IIRC we had to also include <Python.h> for some magical reasons such as
// If we are in a Windows environment, we need to define
// initialization functions for the _C extension
#ifdef _WIN32
#if PY_MAJOR_VERSION < 3
PyMODINIT_FUNC init_C(void) {
// No need to do anything.
// extension.py will run on load
return NULL;
}
#else
PyMODINIT_FUNC PyInit__C(void) {
// No need to do anything.
// extension.py will run on load
return NULL;
}
#endif
#endif
(although maybe this is legacy and could be removed).
Due to those two points, our CMake file currently depends on Python
target_link_libraries(${PROJECT_NAME} PRIVATE ${TORCH_LIBRARIES} ${PNG_LIBRARY} ${JPEG_LIBRARIES} Python3::Python)
but it shouldn't be the case.

@mthrok
Copy link
Contributor

mthrok commented Sep 24, 2020

Yeah, torchtext also depends on Python because they bind functions via PyBind alongside with TorchScript for performance reason.

I think if they can separate registration (binding) and implementation, then things could work nicely.
Here is the diagram to illustrate the idea

Screen Shot 2020-09-24 at 13 06 54

from the doc I am working (internal only)

@mthrok
Copy link
Contributor

mthrok commented Sep 24, 2020

, but IIRC we had to also include <Python.h> for some magical reasons such as

// If we are in a Windows environment, we need to define
// initialization functions for the _C extension
#ifdef _WIN32
#if PY_MAJOR_VERSION < 3
PyMODINIT_FUNC init_C(void) {
// No need to do anything.
// extension.py will run on load
return NULL;
}
#else
PyMODINIT_FUNC PyInit__C(void) {
// No need to do anything.
// extension.py will run on load
return NULL;
}
#endif
#endif

(although maybe this is legacy and could be removed).

At a glance it looks like you can remove Python2 portion at least.

@lanking520
Copy link

@mthrok do you have any plan to fix this? Or can I presume it is okay to temporaily applied the fix I mentioned and will have no side effects?

@stu1130
Copy link
Author

stu1130 commented Sep 25, 2020

Could it be a CMake option like USE_PYTHON? When it is disabled, we skip including <torch/extension.h> with macro.

@mthrok
Copy link
Contributor

mthrok commented Sep 25, 2020

@mthrok do you have any plan to fix this? Or can I presume it is okay to temporaily applied the fix I mentioned and will have no side effects?

I do not have a plan to fix this yet. My work is still proposal phase, and torchvision is more complicated than torchtext or torchaudio.

However if @fmassa's comment

We already solved the torch.ops part in torchvision, and we also depend only on <torch/script.h>

is correct, then I think changing <torch/extension.h> to <torch/torch.h> or <torch/script.h> should suffice, and there should be no side effect. (I could be wrong though)

Searching "pybind11" reveals there is one test code that relies on PyBind11, and that code should include <torch/extension.h> by itself.

@mthrok
Copy link
Contributor

mthrok commented Sep 25, 2020

Could it be a CMake option like USE_PYTHON? When it is disabled, we skip including <torch/extension.h> with macro.

That's what I am working on, but in the case of torchvision, <torch/extension.h> should not be needed, I presume.

@fmassa
Copy link
Member

fmassa commented Sep 28, 2020

Searching "pybind11" reveals there is one test code that relies on PyBind11, and that code should include <torch/extension.h> by itself.

Good catch. This is actually an artifact of testing that should be changed -- we wrapped the C++ models in Python so that we could compare the outputs of the model. But torchvision library itself only uses torch/script.h.

@stu1130
Copy link
Author

stu1130 commented Dec 10, 2020

@fmassa @mthrok is there any update? Thanks

@datumbox
Copy link
Contributor

@stu1130 This is still currently in progress. There are a few PRs that try to minimize the use of torch/script.h see #3087 (comment) and #3146.

@dfalbel
Copy link
Contributor

dfalbel commented Jan 11, 2022

Hi!

Thanks very much for this project! We built a port of torchvision for the R language: https://github.com/mlverse/torchvision and now we want to work on supporting the C++ operations. For that, it would be great if we didn't require users to install Python in order to use our package.

Would you consider a PR avoiding the Python requirement in the builds?

I can successfully build without Python by simply removing the #include <Python.h> statements and adapting the CMake file in order to not link to Python. (see dfalbel@2a81375)

So my approach would be to have a CMake variable, like USE_PYTHON suggested in #2692 (comment) that would conditionally include Python in the target_link_libraries in the CMake file as well as conditionally include Python.h in eg. vision.cpp

#ifndef MOBILE
#include <Python.h>
#endif

And other calls like:

#include "image.h"
#include <ATen/core/op_registration/op_registration.h>
#include <Python.h>

Does that look reasonable? I am no CMake expert, but that seems to be feasible without introducing any breaking change and leaving the default behavior as is.

@fmassa
Copy link
Member

fmassa commented Jan 11, 2022

Hi @dfalbel ,

Thanks for the proposal!

We can accept such a proposal in the CMakeLists (we can actually by default set USE_PYTHON to be false in CMakeLists). But note that ideally we wouldn't change the MOBILE macro name (otherwise we would need to change a few things elsewhere as well, including our internal mobile builds)

This is something that we would like to get fixed for a while now, see #3965

But ideally, we would just remove #include <Python.h> entirely from our C++ files. It is still there because of some Windows-specific issues (which I believe are related to the fact that we use setuptools to build torchvision extensions).

In any case, we should remove the #include <Python.h> by default from the C++ builds.

@dfalbel
Copy link
Contributor

dfalbel commented Jan 11, 2022

Obrigado @fmassa ! Just created a PR in #5190 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants