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

[libtorch] Add new port #8318

Closed
wants to merge 7 commits into from
Closed

[libtorch] Add new port #8318

wants to merge 7 commits into from

Conversation

luncliff
Copy link
Contributor

@luncliff luncliff commented Sep 24, 2019

This PR adds a new port for PyTorch.
Instead of performing local build, the new port extracts prebuilt binaries.

Changes

  • New: libtorch 1.2.0 (C++ part for PyTorch)

Related Issues

Triplets

  • x64-windows (CPU, GPU)
  • x64-osx (CPU)
  • x64-linux (CPU, GPU)

Unavailables

  • *-windows-static
  • *-uwp

Installation

CPU Only

For windows, there are only 1 available triplet.

vcpkg install --triplet x64-windows libtorch

For Linux and OSX, simply install with …

vcpkg install libtorch

GPU (with CUDA)

Notice that CUDA support is only available for Windows/Linux

vcpkg install --triplet x64-windows libtorch[cuda]
vcpkg install --triplet x64-linux   libtorch[cuda]

Usage

Targets of the protobuf and cpuinfo sould be specified.

This is custom installation of the LibTorch for vcpkg:

    find_package(Protobuf   CONFIG REQUIRED)
    find_package(cpuinfo    CONFIG REQUIRED)
    find_package(Torch      CONFIG REQUIRED)

    target_link_libraries(main
    PRIVATE
        # from vcpkg
        protobuf::libprotobuf cpuinfo::clog cpuinfo::cpuinfo

        # from pytorch prebuilt package
        c10 torch caffe2::mkl
    )

@grdowns grdowns changed the title [new port] libtorch [libtorch] Add new port Sep 26, 2019
@cbezault
Copy link
Contributor

In general we don't delete ports. If caffe users should really really really be using this port we should turn caffe into a stub port which prints a deprecation message and redirects the user to this port by having a dependency on this one.

@luncliff
Copy link
Contributor Author

@cbezault Oh, I never thought about the point. Will recover the port soon.

* Download PyTorch 1.2.0 through official url
* SHA512 checksum for each platforms
* Replaced packaged protobuf to that of VcPkg
* use cu100 when feature 'cuda' is requested
@luncliff
Copy link
Contributor Author

luncliff commented Sep 26, 2019

Just fixed the removal of caffe2 0.8.

@cenit
Copy link
Contributor

cenit commented Sep 27, 2019

@luncliff I also propose to leave caffe as an empty port redirecting to this one. Otherwise we would have to check for the reciprocal installation since the two might be incompatible

@luncliff
Copy link
Contributor Author

luncliff commented Sep 27, 2019

@cenit Sorry, I can't get the word, 'empty'.
Is that mean we have to keep ports/caffe2/? or the installation should modify something under installed/{triplet}/{caffe2, libtorch}?

@cenit
Copy link
Contributor

cenit commented Sep 27, 2019

something similar to what happened to ilmbase when it got merged to openexr. Now ilmbase is empty and depends on openexr, so when you install it you are "redirected".
https://github.com/microsoft/vcpkg/tree/master/ports/ilmbase

@luncliff
Copy link
Contributor Author

luncliff commented Sep 27, 2019

@cbezault @cenit Understood. Doing so is indeed easy.
However, the new port(libtorch) is 1.2 and the current caffe2 port is 0.8. I can't sure about their compatibility issue.

When I open the PR I thought they have to be replaced.
But on second thought, I have no confidence that this is the "really really really" case. If possible, I would like to leave them as is.

If I have to make the caffe2 empty, then I will do within days. Just like the ilmbase did.

@cenit
Copy link
Contributor

cenit commented Sep 27, 2019

Since caffe2 just became libtorch from the c++ point of view (and also the python one...), I can't see the point in keeping both. Am I missing something?

also don't be afraid about broken api/abi compatibility. There is no port that needs fixing, and vcpkg itself is very aggressive in updating libraries... so this would be just the perfect way to update an "old" caffe2 port!

@cenit
Copy link
Contributor

cenit commented Sep 27, 2019

for the "history" of caffe2 we also have the x-history command, so don't be afraid in removing the old portfile. There is no real reason to keep it "visible" in the master tree.

* port 'caffe2' depends 'libtorch'
* port 'caffe2' became empty
@luncliff
Copy link
Contributor Author

luncliff commented Sep 27, 2019

@cenit Oh, wow. Learned a new feature!

@cenit
Copy link
Contributor

cenit commented Sep 27, 2019

Also, i’d create an “empty” caffe port (without the 2, yes, linking to caffe2 that links to pytorch). That would be useful for newcomers and to close many issues that otherwise get opened from time to time

@luncliff
Copy link
Contributor Author

I asked about the difference between caffe2 and caffe to my coworker. And his answer was they have much difference unlike their name. So I think adding caffe port in this PR is not appropriate.

Related Issues: caffe

@cenit
Copy link
Contributor

cenit commented Sep 30, 2019

Ok, fair enough

@cenit
Copy link
Contributor

cenit commented Oct 2, 2019

what are the errors on the windows platform? I tried to see also CI:14505 build but it looks empty. Might be that the force-push removed the references to the previous trials, but CI remembered them and declared port already failed. Can you please retrigger the build, just bumping the version inside CONTROL file for example, if you don't know what was wrong?
I am asking this because I'd like to see this PR merged soon and declaring failures as expected with a reason might be necessary

@MVoz
Copy link
Contributor

MVoz commented Oct 4, 2019

install pre-built ?

error

Installing package libtorch[core,cuda]:x64-windows...
The following files are already installed in E:/tools/vcpkg/installed/x64-windows and are in conflict with libtorch:x64-windows
Installed by cpuinfo:x64-windows
    debug/lib/clog.lib
    debug/lib/cpuinfo.lib
    include/clog.h
    include/cpuinfo.h
    lib/clog.lib
    lib/cpuinfo.lib

@luncliff
Copy link
Contributor Author

luncliff commented Oct 4, 2019 via email

* usage: add cpuinfo targets
* portfile checks target architecture and linkage option

by this update, 'x86-windows' and '*-windows-static' must fail.
* the prebuilt package can't be used for the platform :(
@luncliff
Copy link
Contributor Author

luncliff commented Oct 6, 2019

Just updated the files. Now it seems like x64-windows is working well.

@cenit
Copy link
Contributor

cenit commented Oct 6, 2019

Just read the portfile and the initial message better now:

Instead of performing local build, the new port extracts prebuilt binaries.

I am a little bit uncomfortable with it. Is it really necessary? Whenever possible, vcpkg builds library from sources...

@luncliff
Copy link
Contributor Author

luncliff commented Oct 7, 2019

Sorry for late comment, @cenit. I couldn't focus on the issue thesedays.
For the downloading behavior, I agree that the install step should build whenever possible.

However, me and my teammates wanted to ship the package (libtorch 1.2.0) and some other build results in the vcpkg. For the purpose my decision was to place those prebuilt into the installed dir.

If such portfile can't be adopted, I think this PR should be closed.
In my opinion, someone can be motivated to enhance the file if there is an existing one. I wish this libtorch port to be the case.

@cenit
Copy link
Contributor

cenit commented Oct 7, 2019

I can understand your position. Which is reasonable. I am not sure the mods might agree with it or not. The only pre-built ports that you can find on vcpkg are ones that are not providing the source code, and so the built artifacts are the only things available... let’s see here their stance

@jasjuang
Copy link
Contributor

Do we have any decision on this PR? It would be amazing to have libtorch working in vcpkg.

@luncliff
Copy link
Contributor Author

luncliff commented Oct 17, 2019 via email

@cbezault
Copy link
Contributor

@luncliff if you can expand on why you think shipping a pre-built binary is a superior choice in this case I would be interested in hearing it.

@luncliff
Copy link
Contributor Author

luncliff commented Oct 17, 2019

@cbezault I don't think using a prebuilt is a kind of superior choice.
In my opinion, the pros and cons of doing so are like the following.

Pros

Cons

  • Possible linking issues in the future:
    Especially protobuf is the module that made my team suffer from version matching. Port of protobuf and cpuinfo will make progress as time flows, but prebuilt can't handle those issues.

Considering the issue, definitely, the best choice is to build PyTorch from the source. But I think the way requires too much for using the port.

@cbezault
Copy link
Contributor

cbezault commented Oct 17, 2019

It is unfortunate that building will take so long. It seems like the right thing for us to do is to provide a tarball of MKL like Conda does. (We could literally take from conda...).

Once binary caching gets released to the public the build time problem should mostly go away. You could also try to use the beta binary caching feature in your CI.

@luncliff
Copy link
Contributor Author

luncliff commented Oct 17, 2019 via email

@luncliff luncliff closed this Oct 17, 2019
@traversaro
Copy link
Contributor

You could also try to use the beta binary caching feature in your CI.

That is great, I was not aware there was a plan to use those binaries outside your CI system. I thought about using those binary by lurking the CI system logs, but then I desisted as I thought it was only for internal use. Do you have some entry point on how to do that? Thanks in advance.

@cbezault
Copy link
Contributor

It's been a minute since I've had to turn it on but I think there's a --binary-caching flag and there's definitely some sort of environment variable you can set. Note there's still some details that are not added to the hash of the packages. Notably the type of compiler used is not hashed, ports which just just check for system dependencies such as cuda are not hashed correctly, and ports which have dependencies on ports in other triplets like proj4[database] are not hashed correctly.

@cbezault
Copy link
Contributor

Note you will also need to point vcpkg/archives to somewhere all your ci machines can reach (like a network drive)

@luncliff luncliff mentioned this pull request Apr 10, 2021
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants