torch / distro

Torch installation in a self-contained folder
BSD 3-Clause "New" or "Revised" License
555 stars 481 forks source link

Support build torch7 on windows #170

Closed howard0su closed 7 years ago

howard0su commented 7 years ago

Get torch, optim, nn, xlua and their dependency works. largely based on existing windows script.

Tested on Windows 10

howard0su commented 7 years ago

This is still ongoing effort. At least this patch addressed the problems:

  1. Avoid reinstall every dependency when running installdep_win.bat
  2. Build many packages expect
a) trepl: need one patch to support libedit. or we need find another implementation of readline on Windows. howard0su/trepl@e14fd5e

b) luaffifb: already a pull request there to support Windows

c) the following packages should be useful enough: torch, optim, nn, xlua and their dependency.

the next step is getting more packages build include cuXXX packages and evaluate performance.

soumith commented 7 years ago

keep sending the PRs, I love it :)

BTNC commented 7 years ago

Hi @howard0su , thanks for pushing changes for windows. For your reference, I have built and run the whole pack of torch locally successfully, and has put my customization of distro here. I am not using build_windows.bat etc because they are not there when I began port torch to windows, and the merge is not easy.

howard0su commented 7 years ago

@BTNC Sorry I didn't notice your repo. You made more far progress than me. We definitely should collaborate together.

Some high level principle we need agree on:

  1. ultimate goal is merging everything back to main stream. We want the Torch community can benefit and increase the coverage of our users.
  2. keep as much as we can close to what today's Linux build/install/test framework so that this work can be maintained in long run.

I checked your repo and several suggestions:

  1. I like the approach to support MKL instead of just OpenBlas.
  2. CONDA is an interesting ideas. but what's the footprint since this tool is not common like brew on OSX?
  3. I don't like the way to install luajit and luarocks. We should keep it more close to Linux. if there is blocking issue there, let's fix in upstream repo.
  4. I saw several patches, we should consider to push it to its own repo if it make sense. for example, nngraph.patch make totally sense. I would suggest to submit pull request.

Love to hear your suggestions and ideas.

BTNC commented 7 years ago

@howard0su You did not notice my repo because I checked it in just yesterday, haha :-p. I did not check it in earlier since I did not want to check in a semi-manufacture. Actually, I have been continually sending PRs to fix Torch for windows for quite some time. Since it is working in my repo, I think it is time to merge to the mainstream.

  1. I like the approach to support MKL instead of just OpenBlas.

Locally I am using MKL downloaded from intel website. conda does provide MKL from the intel channel, however I did not investigate how to incorporate it into torch.

CONDA is an interesting ideas. but what's the footprint since this tool is not common like brew on OSX?

brew comes along with OSX while windows does not come along with any package manager. It is a common tool for anaconda python users on windows, is actively maintained and I believe it is currently the best package manager for windows.

  1. I don't like the way to install luajit and luarocks. We should keep it more close to Linux. if there is blocking issue there, let's fix in upstream repo.

I want to keep luajit and luarocks up-to-date, and be flexible for my use locally. The only thing I need from luajit-rocks is the rocks server config. This part does not need to be merged to official master branch. It is just a demonstration how I am using a self contained luajit luarocks locally.

  1. I saw several patches, we should consider to push it to its own repo if it make sense. for example, nngraph.patch make totally sense. I would suggest to submit pull request.

I have been sending patches to fix torch for windows for quite some time. These patches are the remaining ones to make the whole pack of torch work on windows. And I am continuing sending PRs for them. Hopefully, I do not need to keep those patches.

I prefer the following changes for the official master:

  1. do not have separate rockspecs, change the rockspecs in packages to fit windows usage.
  2. do not hide the exe installations from the user, let the user decide which to install and where to install. For example, there are 3 kinds of vc tools. Users only need to install vc tools, git, cmake and conda (if decide to use conda). There is no need to install 7z since luarocks has one in its tools.
  3. Use conda to manage dependencies. It is the best choice I can think of on windows.
howard0su commented 7 years ago

Why not you submit the patch to override what I am doing? So that we can use your as the base for next step.

BTNC commented 7 years ago

Why not you submit the patch to override what I am doing?

Do you mean submit my customized distro to offical master? I can do that if no objection, but that will result in a ugly distro with two ways of installing torch on windows. Though they can finally be merged as one.

I am think of changing official master to use conda if no objection for the first step. Tools like 7z git cmake etc can also be installed by conda. And I do want to rename build_windows.bat etc to be consistent with linux scripts, i am not sure if this is acceptable.

soumith commented 7 years ago

i support both changes proposed by @BTNC . conda is a much more pleasant and consistent experience on windows

howard0su commented 7 years ago

@soumith we need your option? At least I think moving to conda is good approach.

howard0su commented 7 years ago

I played a bit on your repo. here is some feedback:

  1. it fails in my box. I didn't get chance to figure out why. I will find out.
  2. we can detect VC tools via checking environment variables. I would prefer that over asking user to run the command from a special command line prompt.
  3. better we don't git clone other code. rather using the in-tree ones to keep consistency. understand you requirement to keep flexible on local for development. maybe make it as an option?
BTNC commented 7 years ago
  1. it fails in my box. I didn't get chance to figure out why. I will find out.

Could you post the console output, so that I can tell what step it fails on.

  1. we can detect VC tools via checking environment variables. I would prefer that over asking user to run the command from a special command line prompt.

How about users with multiple versions of MSVC? In the README, I suggest "luarocks install should be run in "Visual Studio Native Tools Command Prompt" or consoles with MSVC setup". So I prefer the user to choose what console to use as long as MSVC environment is setup in the console.

  1. better we don't git clone other code. rather using the in-tree ones to keep consistency. understand you requirement to keep flexible on local for development. maybe make it as an option?

I simply want Torch distribution to be clean without non-Torch libraries. Non-Torch libraries will be git cloned or downloaded while Torch is installed. Unless the user remove non-Torch libraries manually, they will be there so the next time user want to reinstall Torch, those libraries will not be downloaded again. However, I do think it is good to have an option to turn off checking for updates(i.e. git pull) for non-Torch libraries.

I have submitted PRs for most remaining patches. As long as they are accepted and the master distro is updated, I will send a new PR to push my customized distro to master per your suggestion. And we can try to merge the good parts of the two win-install methods together.

BTNC commented 7 years ago

@howard0su I found that I hard coded some path in install-deps.bat, that might be the reason for your failure. Sorry.

howard0su commented 7 years ago

@BTNC it turns out there is a missing file msvcbuild.bat 176 call msvcbuild.bat || goto :FAIL 'msvcbuild.bat' is not recognized as an internal or external command, operable program or batch file.

BTNC commented 7 years ago

@howard0su msvcbuild.bat should be in luajit/src, the file can not be found probably because luajit is not properly cloned or somehow install-deps.bat goes into wrong directory.