Skip to content

Conversation

@larryliu0820
Copy link
Contributor

@larryliu0820 larryliu0820 commented Oct 7, 2021

Summary:

Fixing windows build for torchvision. In csrc/vision.cpp, since PyMODINIT_FUNC depends on Python.h I added the same condition for PyMODINIT_FUNC as the one for import <PyTorch.h>.

Differential Revision: D31488734

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D31488734

…ytorch#4571)

Summary:
Pull Request resolved: pytorch#4571

Fixing windows build for `torchvision` when `MOBILE` is defined. In `csrc/vision.cpp`, since `PyMODINIT_FUNC` depends on `Python.h` I added the same condition for `PyMODINIT_FUNC` as the one for `import <PyTorch.h>`.

Reviewed By: malfet

Differential Revision: D31488734

fbshipit-source-id: 88f24ccffbc3e05f0b7ecbb63dff16d41c8b5592
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D31488734

@larryliu0820 larryliu0820 force-pushed the export-D31488734-to-fbsync branch from 2cff471 to e0df602 Compare October 9, 2021 01:32
facebook-github-bot pushed a commit that referenced this pull request Oct 9, 2021
…4571)

Summary:
Pull Request resolved: #4571

Fixing windows build for `torchvision` when `MOBILE` is defined. In `csrc/vision.cpp`, since `PyMODINIT_FUNC` depends on `Python.h` I added the same condition for `PyMODINIT_FUNC` as the one for `import <PyTorch.h>`.

Reviewed By: malfet

Differential Revision: D31488734

fbshipit-source-id: fd52a9ab161288d9c8bf3b9e0ab9da856a4ebadb
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM. I'm not going to approve as we can't merge on fbsync branch. Feel free to merge on FBcode and we'll take care of the rest of the syncs.

return NULL;
}
#endif
#endif // !defined(MOBILE) && defined(_WIN32)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an optional nitpick. I checked the code-base and we typically do't mark endifs like this:

Suggested change
#endif // !defined(MOBILE) && defined(_WIN32)
#endif

@larryliu0820 larryliu0820 changed the title [PyTorch] Fix windows build for torchvision (#440) [PyTorch] Fix windows build for torchvision Oct 12, 2021
@datumbox datumbox closed this Oct 12, 2021
datumbox pushed a commit to datumbox/vision that referenced this pull request Oct 26, 2021
…ytorch#4571)

Summary:
Pull Request resolved: pytorch#4571

Fixing windows build for `torchvision` when `MOBILE` is defined. In `csrc/vision.cpp`, since `PyMODINIT_FUNC` depends on `Python.h` I added the same condition for `PyMODINIT_FUNC` as the one for `import <PyTorch.h>`.

Reviewed By: malfet

Differential Revision: D31488734

fbshipit-source-id: fd52a9ab161288d9c8bf3b9e0ab9da856a4ebadb
datumbox added a commit that referenced this pull request Oct 26, 2021
…4571) (#4742)

Summary:
Pull Request resolved: #4571

Fixing windows build for `torchvision` when `MOBILE` is defined. In `csrc/vision.cpp`, since `PyMODINIT_FUNC` depends on `Python.h` I added the same condition for `PyMODINIT_FUNC` as the one for `import <PyTorch.h>`.

Reviewed By: malfet

Differential Revision: D31488734

fbshipit-source-id: fd52a9ab161288d9c8bf3b9e0ab9da856a4ebadb

Co-authored-by: Mengwei Liu <[email protected]>
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
…ytorch#4571) (pytorch#4742)

Summary:
Pull Request resolved: pytorch#4571

Fixing windows build for `torchvision` when `MOBILE` is defined. In `csrc/vision.cpp`, since `PyMODINIT_FUNC` depends on `Python.h` I added the same condition for `PyMODINIT_FUNC` as the one for `import <PyTorch.h>`.

Reviewed By: malfet

Differential Revision: D31488734

fbshipit-source-id: fd52a9ab161288d9c8bf3b9e0ab9da856a4ebadb

Co-authored-by: Mengwei Liu <[email protected]>
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.

3 participants