Improve Windows Compatibility(for csrc/scripts) by peterjc123 · Pull Request #2941 · pytorch/pytorch

peterjc123 · 2017-10-03T02:09:29Z

Win64 support for csrc and scripts

soumith · 2017-10-03T03:28:30Z

@peterjc123 is there any chance you could rebase your commits instead of making merge commits? this branch cannot be merged into master, it still has conflicts.

soumith · 2017-10-03T03:28:36Z

@pytorchbot add to whitelist

soumith · 2017-10-03T03:29:05Z

if you think rebasing is too difficult, let me know i can try to do it.

peterjc123 · 2017-10-03T03:40:20Z

I don't know where the conflicts are.

peterjc123 · 2017-10-03T05:05:55Z

This time it's done using git diff and git apply. I think it should be rebased now.

tools/setup_helpers/cuda.py

 from .env import check_env_flag

+LINUX_HOME = '/usr/local/cuda'
+WINDOWS_HOME = 'C:/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v8.0'


torch/cuda/__init__.py

 _original_pid = False
 _cudart = None

+CUDA_WINDOWS_LIB = 'cudart64_80'


torch/serialization.py

 DEFAULT_PROTOCOL = 2

-LONG_SIZE = struct.Struct('=l').size
+LONG_SIZE = struct.Struct('=Q').size


torch/utils/serialization/read_lua_file.py


 LuaFunction = namedtuple('LuaFunction', ['size', 'dumped', 'upvalues'])

+LONGLONG_TYPECODE = 'q' if sys.version[0] == '3' else 'l'


apaszke

What's the story for multiprocessing? Can you please write up a small summary explaining how does it work on Windows and if we still have two modes there?

setup.py


+# Debug option for Windows
+if IS_WINDOWS:
+    extra_link_args.append('/DEBUG:FULL')


setup.py

+    ATEN_LIB = os.path.join(lib_path, 'ATen.lib')
+    _C_LIB = 'build/temp.win-amd64-' + str(sys.version_info[0]) + '.' + str(
+        sys.version_info[1]) + '/Release/torch/csrc/_C.cp' + str(
+            sys.version_info[0]) + str(sys.version_info[1]) + '-win_amd64.lib'


setup.py

                           THCUNN_LIB,
                           make_relative_rpath('../lib'),
-                       ]
+                       ] + [_C_LIB] if _C_LIB is not None else []


test/common.py

            expected_file += "-" + subname
-        expected_file += ".expect"
+        if sys.platform == 'win32' and os.path.exists(expected_file + '.expect.win'):
+            expected_file += ".expect.win"


test/test_multiprocessing.py

+        self.daemon = True
+
+    def run(self):
+        self.tensor.add_(3)


torch/csrc/autograd/init.cpp

+    __assume(0);
+#else
    __builtin_unreachable();
+#endif


torch/csrc/cuda/AutoGPU.h

 #include "torch/csrc/utils/auto_gpu.h"

+#if defined(WITH_CUDA) && defined(_MSC_VER)
+class THP_CLASS THCPAutoGPU : public AutoGPU {


torch/csrc/generic/StorageSharing.cpp

  size_t view_size =  (size_t)THPUtils_unpackLong(_view_size);

-  long device = THPUtils_unpackLong(_device);
+  int device = (int) THPUtils_unpackLong(_device);


torch/csrc/generic/methods/Tensor.cwrap

+  int dim = 0;
+#else
+  int64_t dim = 0;
+#endif


torch/utils/serialization/read_lua_file.py

+            return self._read(LONGLONG_TYPECODE)
        elif self.long_size is 8:
-            return self._read('q')
+            return self._read(LONGLONG_TYPECODE)


peterjc123 · 2017-10-03T13:14:30Z

@apaszke About multiprocessing, only spawn is supported. Shared file mapping(File sharing) was used to share data between processes.

apaszke · 2017-10-03T13:16:27Z

But are there still two different methods (file system + file descriptor)? Or do they dispatch to the same thing?

peterjc123 · 2017-10-03T13:55:06Z

@apaszke File descriptor is not supported in Windows. So only file system is used.

soumith · 2017-10-05T01:34:22Z

@apaszke can you check that all the changes you requested are done?

peterjc123 · 2017-10-06T01:36:19Z

@fmassa Yes, and the native size of 'l' is 4 in Windows but 8 in Unix. So there 'll be no option for py2 in Windows.

fmassa · 2017-10-06T11:29:23Z

Does this mean that Windows Py2 people won't be able to load legacy models?

peterjc123 · 2017-10-06T12:07:21Z

@fmassa Yes, however, nvcc doesn't compile with any compiler other than MSVC in Windows. So that one is not the only block for PyTorch on windows py2.

peterjc123 · 2017-10-10T03:06:03Z

Are there more comments? I think that I have covered all the points above except for those can't fix.

peterjc123 · 2017-11-08T17:26:24Z

@apaszke Fixed. Very sorry for my misunderstanding.

apaszke

No need to apologise, my comment was unclear. Sorry for that. Last fix and it should be good to go

torch/csrc/cudnn/Conv.cpp

        ws.data,
        ws.size));
-    return best_algo = getBestAlgorithm<cudnnConvolutionBwdFilterAlgoPerf_t>(perfResults.release(), deterministic, n_algo);
+    return best_algo = getBestAlgorithm<cudnnConvolutionBwdFilterAlgoPerf_t>(perfResults.get(), deterministic, n_algo);


torch/csrc/cudnn/Conv.cpp

        out,
        1,
-        &algoCount,
+        &algoCount.get(),


peterjc123 · 2017-11-08T17:47:46Z

@apaszke It finally passes the build phase.

apaszke

Looks good! Thanks a lot!

apaszke · 2017-11-08T18:52:08Z

Alright, landed in master! Thank you so much!!

soumith · 2017-11-08T19:39:06Z

woooohooo!!! finally!

fmassa · 2017-11-08T21:29:57Z

Looks like this broke compilation on clang 8.0.0 with error messages like

torch/csrc/autograd/functions/init.cpp:222:29: error: address of overloaded function 'getValueAttr' does not match required type '_object *(_object *, void *)'
  {(char*)"groups", (getter)getValueAttr<ConvBackwardBackward, int, ConvParams,
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
torch/csrc/autograd/functions/init.cpp:99:11: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'Convert'
PyObject* getValueAttr(PyObject* obj, void* _unused)
          ^

lantiga · 2017-11-09T00:31:58Z

I'm on this

lantiga · 2017-11-09T00:34:21Z

Ok, never mind, @ezyang already has the patch (#3573)

…ipt. --- How does the current code subsume all detections in the deleted `nccl.py`? - The dependency of `USE_NCCL` on the OS and `USE_CUDA` is handled as dependency options in `CMakeLists.txt`. - The main NCCL detection happens in [FindNCCL.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/Modules/FindNCCL.cmake), which is called by [nccl.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/External/nccl.cmake). When `USE_SYSTEM_NCCL` is false, the previous Python code defer the detection to `find_package(NCCL)`. The change in `nccl.cmake` retains this. - `USE_STATIC_NCCL` in the previous Python code simply changes the name of the detected library. This is done in `IF (USE_STATIC_NCCL)`. - Now we only need to look at how the lines below line 20 in `nccl.cmake` are subsumed. These lines list paths to header and library directories that NCCL headers and libraries may reside in and try to search these directories for the key header and library files in turn. These are done by `find_path` for headers and `find_library` for the library files in `FindNCCL.cmake`. * The call of [find_path](https://cmake.org/cmake/help/v3.8/command/find_path.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for headers in `<prefix>/include` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. Like the Python code, this commit sets `CMAKE_PREFIX_PATH` to search for `<prefix>` in `NCCL_ROOT_DIR` and home to CUDA. `CMAKE_SYSTEM_PREFIX_PATH` includes the standard directories such as `/usr/local` and `/usr`. `NCCL_INCLUDE_DIR` is also specifically handled. * Similarly, the call of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for libraries in directories including `<prefix>/lib` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. But it also handles the edge cases intended to be solved in the Python code more properly: - It only searches for `<prefix>/lib64` (and `<prefix>/lib32`) if it is appropriate on the system. - It only searches for `<prefix>/lib/<arch>` for the right `<arch>`, unlike the Python code searches for `lib/<arch>` in a generic way (e.g., the Python code searches for `/usr/lib/x86_64-linux-gnu` but in reality systems have `/usr/lib/x86_64-some-customized-name-linux-gnu`, see https://unix.stackexchange.com/a/226180/38242 ). --- Regarding for relevant issues: - pytorch#12063 and pytorch#2877: These are properly handled, as explained in the updated comment. - pytorch#2941 does not changes NCCL detection specifically for Windows (it changed CUDA detection). - b7e258f A versioned library detection is added, but the order is reversed: The unversioned library becomes preferred. This is because normally unversioned libraries are linked to versioned libraries and preferred by users, and local installation by users are often unversioned. Like the document of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) suggests: > When using this to specify names with and without a version suffix, we recommend specifying the unversioned name first so that locally-built packages can be found before those provided by distributions.

…ipt. (#22930) Summary: --- How does the current code subsume all detections in the deleted `nccl.py`? - The dependency of `USE_NCCL` on the OS and `USE_CUDA` is handled as dependency options in `CMakeLists.txt`. - The main NCCL detection happens in [FindNCCL.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/Modules/FindNCCL.cmake), which is called by [nccl.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/External/nccl.cmake). When `USE_SYSTEM_NCCL` is false, the previous Python code defer the detection to `find_package(NCCL)`. The change in `nccl.cmake` retains this. - `USE_STATIC_NCCL` in the previous Python code simply changes the name of the detected library. This is done in `IF (USE_STATIC_NCCL)`. - Now we only need to look at how the lines below line 20 in `nccl.cmake` are subsumed. These lines list paths to header and library directories that NCCL headers and libraries may reside in and try to search these directories for the key header and library files in turn. These are done by `find_path` for headers and `find_library` for the library files in `FindNCCL.cmake`. * The call of [find_path](https://cmake.org/cmake/help/v3.8/command/find_path.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for headers in `<prefix>/include` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. Like the Python code, this commit sets `CMAKE_PREFIX_PATH` to search for `<prefix>` in `NCCL_ROOT_DIR` and home to CUDA. `CMAKE_SYSTEM_PREFIX_PATH` includes the standard directories such as `/usr/local` and `/usr`. `NCCL_INCLUDE_DIR` is also specifically handled. * Similarly, the call of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for libraries in directories including `<prefix>/lib` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. But it also handles the edge cases intended to be solved in the Python code more properly: - It only searches for `<prefix>/lib64` (and `<prefix>/lib32`) if it is appropriate on the system. - It only searches for `<prefix>/lib/<arch>` for the right `<arch>`, unlike the Python code searches for `lib/<arch>` in a generic way (e.g., the Python code searches for `/usr/lib/x86_64-linux-gnu` but in reality systems have `/usr/lib/x86_64-some-customized-name-linux-gnu`, see https://unix.stackexchange.com/a/226180/38242 ). --- Regarding for relevant issues: - #12063 and #2877: These are properly handled, as explained in the updated comment. - #2941 does not changes NCCL detection specifically for Windows (it changed CUDA detection). - b7e258f A versioned library detection is added, but the order is reversed: The unversioned library becomes preferred. This is because normally unversioned libraries are linked to versioned libraries and preferred by users, and local installation by users are often unversioned. Like the document of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) suggests: > When using this to specify names with and without a version suffix, we recommend specifying the unversioned name first so that locally-built packages can be found before those provided by distributions. Pull Request resolved: #22930 Differential Revision: D16440275 Pulled By: ezyang fbshipit-source-id: 11fe80743d4fe89b1ed6f96d5d996496e8ec01aa

…ipt. (#22930) Summary: --- How does the current code subsume all detections in the deleted `nccl.py`? - The dependency of `USE_NCCL` on the OS and `USE_CUDA` is handled as dependency options in `CMakeLists.txt`. - The main NCCL detection happens in [FindNCCL.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/Modules/FindNCCL.cmake), which is called by [nccl.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/External/nccl.cmake). When `USE_SYSTEM_NCCL` is false, the previous Python code defer the detection to `find_package(NCCL)`. The change in `nccl.cmake` retains this. - `USE_STATIC_NCCL` in the previous Python code simply changes the name of the detected library. This is done in `IF (USE_STATIC_NCCL)`. - Now we only need to look at how the lines below line 20 in `nccl.cmake` are subsumed. These lines list paths to header and library directories that NCCL headers and libraries may reside in and try to search these directories for the key header and library files in turn. These are done by `find_path` for headers and `find_library` for the library files in `FindNCCL.cmake`. * The call of [find_path](https://cmake.org/cmake/help/v3.8/command/find_path.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for headers in `<prefix>/include` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. Like the Python code, this commit sets `CMAKE_PREFIX_PATH` to search for `<prefix>` in `NCCL_ROOT_DIR` and home to CUDA. `CMAKE_SYSTEM_PREFIX_PATH` includes the standard directories such as `/usr/local` and `/usr`. `NCCL_INCLUDE_DIR` is also specifically handled. * Similarly, the call of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for libraries in directories including `<prefix>/lib` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. But it also handles the edge cases intended to be solved in the Python code more properly: - It only searches for `<prefix>/lib64` (and `<prefix>/lib32`) if it is appropriate on the system. - It only searches for `<prefix>/lib/<arch>` for the right `<arch>`, unlike the Python code searches for `lib/<arch>` in a generic way (e.g., the Python code searches for `/usr/lib/x86_64-linux-gnu` but in reality systems have `/usr/lib/x86_64-some-customized-name-linux-gnu`, see https://unix.stackexchange.com/a/226180/38242 ). --- Regarding for relevant issues: - pytorch/pytorch#12063 and pytorch/pytorch#2877: These are properly handled, as explained in the updated comment. - pytorch/pytorch#2941 does not changes NCCL detection specifically for Windows (it changed CUDA detection). - b7e258f81ef61d19b884194cdbcd6c7089636d46 A versioned library detection is added, but the order is reversed: The unversioned library becomes preferred. This is because normally unversioned libraries are linked to versioned libraries and preferred by users, and local installation by users are often unversioned. Like the document of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) suggests: > When using this to specify names with and without a version suffix, we recommend specifying the unversioned name first so that locally-built packages can be found before those provided by distributions. Pull Request resolved: pytorch/pytorch#22930 Differential Revision: D16440275 Pulled By: ezyang fbshipit-source-id: 11fe80743d4fe89b1ed6f96d5d996496e8ec01aa

peterjc123 mentioned this pull request Oct 3, 2017

Improve Windows Compatibility(for csrc) #2801

Closed

peterjc123 force-pushed the csrc_script_fix branch from fa54164 to a2035e6 Compare October 3, 2017 05:03

fmassa reviewed Oct 3, 2017

View reviewed changes

torch/cuda/__init__.py Outdated

_original_pid = False

_cudart = None

CUDA_WINDOWS_LIB = 'cudart64_80'

This comment was marked as off-topic.

Sign in to view

fmassa reviewed Oct 3, 2017

View reviewed changes

torch/serialization.py Outdated

DEFAULT_PROTOCOL = 2

LONG_SIZE = struct.Struct('=l').size

LONG_SIZE = struct.Struct('=Q').size

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

fmassa reviewed Oct 3, 2017

View reviewed changes

torch/utils/serialization/read_lua_file.py Outdated

LuaFunction = namedtuple('LuaFunction', ['size', 'dumped', 'upvalues'])

LONGLONG_TYPECODE = 'q' if sys.version[0] == '3' else 'l'

This comment was marked as off-topic.

Sign in to view

apaszke reviewed Oct 3, 2017

View reviewed changes

peterjc123 force-pushed the csrc_script_fix branch 7 times, most recently from a0bd882 to fb9d3aa Compare October 15, 2017 03:26

peterjc123 force-pushed the csrc_script_fix branch 3 times, most recently from 22ea9e8 to 87c8d54 Compare October 20, 2017 09:38

last fixes

2459b3c

apaszke reviewed Nov 8, 2017

View reviewed changes

mingwei-liu added 2 commits November 9, 2017 01:29

last fixes really

1d3b46e

last fixes really true

0008a18

apaszke reviewed Nov 8, 2017

View reviewed changes

torch/csrc/cudnn/Conv.cpp Outdated

out,

1,

&algoCount,

&algoCount.get(),

This comment was marked as off-topic.

Sign in to view

mingwei-liu added 2 commits November 9, 2017 01:40

last fixes really true?

8e7f2d1

remove the additional best_algo

bb39f23

apaszke approved these changes Nov 8, 2017

View reviewed changes

apaszke merged commit aa91193 into pytorch:master Nov 8, 2017

peterjc123 deleted the csrc_script_fix branch November 9, 2017 04:50

ekostem mentioned this pull request Nov 10, 2017

Building from source error: command 'gcc' failed with exit status 1 #3628

Closed

peterjc123 mentioned this pull request Dec 18, 2017

Add build support for Python 2.7 using MSVC #4226

Merged

5 tasks

yf225 mentioned this pull request Dec 20, 2017

Missing components / tests on Windows #4092

Closed

13 tasks

ezyang added the open source label Jun 24, 2019

ezyang mentioned this pull request Jul 3, 2019

Let CMake handle NCCL detection instead of our handcrafted Python script. #22480

Closed

xuhdev mentioned this pull request Jul 12, 2019

Let CMake handle NCCL detection instead of our handcrafted Python script. #22818

Closed

xuhdev mentioned this pull request Jul 16, 2019

Let CMake handle NCCL detection instead of our handcrafted Python script. #22930

Closed


		LuaFunction = namedtuple('LuaFunction', ['size', 'dumped', 'upvalues'])

		LONGLONG_TYPECODE = 'q' if sys.version[0] == '3' else 'l'

Conversation

peterjc123 commented Oct 3, 2017

Uh oh!

soumith commented Oct 3, 2017

Uh oh!

soumith commented Oct 3, 2017

Uh oh!

soumith commented Oct 3, 2017

Uh oh!

peterjc123 commented Oct 3, 2017

Uh oh!

peterjc123 commented Oct 3, 2017

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

apaszke left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

peterjc123 commented Oct 3, 2017

Uh oh!

apaszke commented Oct 3, 2017

Uh oh!

peterjc123 commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soumith commented Oct 5, 2017

Uh oh!

peterjc123 commented Oct 6, 2017

Uh oh!

fmassa commented Oct 6, 2017

peterjc123 commented Oct 3, 2017 •

edited

Loading

peterjc123 commented Oct 10, 2017 •

edited

Loading