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

Remove include Python.h #8413

Merged
merged 5 commits into from
May 9, 2024
Merged

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented May 8, 2024

Closes #3965

For some reason, on Windows we need to define the PyInit_video_reader PyInit_image and PyInit__C symbols, otherwise the windows linker fails with something like

LINK : error LNK2001: unresolved external symbol PyInit__C
%SRC_DIR%\build\temp.win-amd64-cpython-38\Release\Jenkins\Miniconda3\conda-bld\torchvision_1715167042718\work\torchvision\csrc\ops\autocast\_C.lib : fatal error LNK1120: 1 unresolved externals

To avoid that we were defining the symbols as

PyMODINIT_FUNC PyInit__C(void) {
  // No need to do anything.
  return nullptr;
}

But that PyMODINIT_FUNC forced us to include Python.h and take a dependency on Libpython. (BTW, we probably never needed to link to libpython like we did in the CMakeFile.txt, we probably only needed the headers, but that's another story.)

This PR changes this PyMODINIT_FUNC into a void*, which seems to work. In reality, PyMODINIT_FUNC is PyObject_t*.

Copy link

pytorch-bot bot commented May 8, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8413

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit e50bdb7 with merge base e4d2d1a (image):

NEW FAILURE - The following job has failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@NicolasHug NicolasHug changed the title [WIP] Remove include Python.h Remove include Python.h May 9, 2024
@NicolasHug NicolasHug marked this pull request as ready for review May 9, 2024 09:31
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Stamp

@NicolasHug NicolasHug merged commit 5367021 into pytorch:main May 9, 2024
87 of 89 checks passed
@NicolasHug NicolasHug deleted the remove_include_Pythonh branch May 9, 2024 09:43
Copy link

github-actions bot commented May 9, 2024

Hey @NicolasHug!

You merged this PR, but no labels were added.
The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request May 20, 2024
Reviewed By: vmoens

Differential Revision: D57565241

fbshipit-source-id: 47c428651d5ca5a9d33a6c83fa5d64b08ff23a06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove need of include Python.h in vision.cpp
3 participants