Skip to content

Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windows #9491

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.

Sign up for GitHub

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

Jump to bottom
Closed
peterjc123 wants to merge 1 commit into from
Closed

Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windows #9491

peterjc123 wants to merge 1 commit into from

Conversation

@peterjc123
Copy link
Collaborator

@peterjc123 peterjc123 commented Jul 17, 2018

Fixes #9092.

@peterjc123 peterjc123 changed the title Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windows [WIP] Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windows Jul 17, 2018
@peterjc123 peterjc123 mentioned this pull request Jul 18, 2018
Closed
@peterjc123
Copy link
Collaborator Author

peterjc123 commented Jul 18, 2018 •
edited
Loading

@ezyang The work is almost done, except one change needed in the onnx repo.

@peterjc123
Copy link
Collaborator Author

peterjc123 commented Jul 18, 2018

@ezyang Test passed locally on CPU build and the CUDA build should pass too. Could you spare some time to review this?

@peterjc123 peterjc123 changed the title [WIP] Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windows Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windows Jul 18, 2018
ezyang
ezyang approved these changes Jul 19, 2018
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

This is amazingly concise! Thank you!

@peterjc123
Copy link
Collaborator Author

peterjc123 commented Jul 19, 2018

Oh no, it breaks again.

@yf225
Copy link
Contributor

yf225 commented Jul 19, 2018

@pytorchbot retest this please

@yf225
Copy link
Contributor

yf225 commented Jul 19, 2018

@peterjc123 Do you think we can submit a PR to ONNX repo in order to merge this soon? We are thinking of doing the 0.4.1 release by the end of this week or on Monday. If this still needs time we can try to just patch up the 0.4.1 branch to make Windows CUDA 8 build work. Thanks!

cc. @soumith

@peterjc123
Copy link
Collaborator Author

peterjc123 commented Jul 20, 2018 •
edited
Loading

@soumith This PR does not require the code change in ONNX. Actually, since the code in JIT is changing every day, the code here get quickly broken. I guess we should merge this ASAP. Otherwise, I'll
fall into the loop of changing and rebasing.

@peterjc123 peterjc123 mentioned this pull request Jul 20, 2018
Merged
@peterjc123 peterjc123 force-pushed the export_fix branch 2 times, most recently from 0fcb92b to ad906f2 Compare July 21, 2018 04:37
@peterjc123
Copy link
Collaborator Author

peterjc123 commented Jul 21, 2018 •
edited
Loading

It seems that all the issues has been solved except this one

python_nn_functions.obj : error LNK2001: unresolved external symbol "public: static void __cdecl torch::jit::tracer::ArgumentStash::stashIntListElem(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,unsigned __int64,unsigned __int64,struct torch::autograd::Variable const &)" (?stashIntListElem@ArgumentStash@tracer@jit@torch@@SAXAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@_K1AEBUVariable@autograd@4@@Z)

@apaszke Do you have any idea on this? It is annotated as static, but seems to be referenced in torch\csrc\utils\python_arg_parser.h.

@peterjc123
Copy link
Collaborator Author

peterjc123 commented Jul 21, 2018

@pytorchbot retest this please

facebook-github-bot
facebook-github-bot reviewed Jul 21, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang ezyang mentioned this pull request Jul 22, 2018
Closed
ezyang pushed a commit to ezyang/pytorch that referenced this pull request Jul 22, 2018
@peterjc123 @facebook-github-bot
Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windo…
9b67787 
…ws (pytorch#9693)

Summary:
Pull Request resolved: pytorch#9693

Fixes pytorch#9092.
Pull Request resolved: pytorch#9491

Differential Revision: D8946850

fbshipit-source-id: f8e053542b573c0d778f8be62773cc761e090c77
zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 23, 2018
@peterjc123 @facebook-github-bot
Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windo…
dc334e8 
…ws (#9491) (#9491)

Summary:
Fixes #9092.
Pull Request resolved: pytorch/pytorch#9491
Pull Request resolved: pytorch/pytorch#9693

Differential Revision: D8946850

Pulled By: ezyang

fbshipit-source-id: bd816f459ab70f6b4a0983305a1ce341bb633707
@peterjc123 peterjc123 deleted the export_fix branch July 23, 2018 15:14
jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 30, 2018
@peterjc123 @jramseyer
Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windo…
516d86c 
…ws (pytorch#9491) (pytorch#9491)

Summary:
Fixes pytorch#9092.
Pull Request resolved: pytorch#9491
Pull Request resolved: pytorch#9693

Differential Revision: D8946850

Pulled By: ezyang

fbshipit-source-id: bd816f459ab70f6b4a0983305a1ce341bb633707
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@peterjc123
Remove CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS and fix CUDA 8 build on Windo…
fb10253 
…ws (pytorch#9491) (pytorch#9491)

Summary:
Fixes pytorch#9092.
Pull Request resolved: pytorch#9491
Pull Request resolved: pytorch#9693

Differential Revision: D8946850

Pulled By: ezyang

fbshipit-source-id: bd816f459ab70f6b4a0983305a1ce341bb633707
@peterjc123 peterjc123 mentioned this pull request Jun 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ezyang ezyang ezyang approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

@anderspapitto anderspapitto Awaiting requested review from anderspapitto

@bddppq bddppq Awaiting requested review from bddppq

@dzhulgakov dzhulgakov Awaiting requested review from dzhulgakov

@houseroad houseroad Awaiting requested review from houseroad

@jamesr66a jamesr66a Awaiting requested review from jamesr66a

@smessmer smessmer Awaiting requested review from smessmer

@Yangqing Yangqing Awaiting requested review from Yangqing

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CUDA 8 build failed on Windows

4 participants

@peterjc123 @yf225 @ezyang @facebook-github-bot