Skip to content

Conversation

@dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Jan 27, 2022

Hi everyone!

Thanks for this amazing package!

I'm working on a port of pytorch_sparse for the R language so users can benefit of the sparse operators together with torch. The current version lives in this repo.
It would be nice though if one could use the C++ library without linking to Python.

This PR adds a CMake option WITH_PYTHON that when set to OFF allows the package to be built entirely without linking to Python. By default, we tried to keep the same behavior, link to Python and include Python.h headers.

@dfalbel
Copy link
Contributor Author

dfalbel commented Jan 27, 2022

I think the error to install pytorch is unrelated to this change. But we still might need to modify:

define_macros = []

to add the WITH_PYTHON definition.

Copy link
Owner

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. This looks good. Left a few comments.

As you pointed out, we also need to set WITH_PYTHON by default in setup.py.

@@ -0,0 +1,9 @@

#include <torch/torch.h>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we now include torch.h rather than extension.h?

Copy link
Contributor Author

@dfalbel dfalbel Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extension.h includes Python.h so at least in the non-python builds we can't have it.

https://github.com/pytorch/pytorch/blob/e88c999da38f30da123e14bdf0898bc475c8d04a/torch/extension.h#L1-L6

However the library doesn't seem to use any <torch/python.h> features as operators seem to be exported via RegisterOperators.

We could still keep including extensions.h in the Python builds and only remove them in the non-python builds if you think it's safer. What do you think?

#include <torch/torch.h>

// for getpid()
#ifdef _WIN32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need those?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, including torch/python.h leaks the getpid() headers. Those headers are not included in torch/torch.h so we had to manually add them as getpid() is used for random seed initialization in a few operators.

Since getpid() is OS specific we have to include different headers on Unix and Windows.

@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2022

Codecov Report

Merging #196 (c6c2eb3) into master (fe8c3ce) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #196   +/-   ##
=======================================
  Coverage   72.32%   72.32%           
=======================================
  Files          28       28           
  Lines        1120     1120           
=======================================
  Hits          810      810           
  Misses        310      310           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe8c3ce...c6c2eb3. Read the comment docs.

Copy link
Owner

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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