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

[tensorpipe] create a new port #16472

Merged
merged 23 commits into from
May 18, 2021
Merged

[tensorpipe] create a new port #16472

merged 23 commits into from
May 18, 2021

Conversation

luncliff
Copy link
Contributor

@luncliff luncliff commented Mar 1, 2021

What does your PR fix?

There was no port request for this project.

This is one of the 3rd party libraries for the PyTorch project. The PR will be used for future support of the libtorch port.

Which triplets are supported/not supported? Have you updated the CI baseline?

According to the project's CI configuration, its target is linux, osx. And there are no sources for windows, uwp triplets.

  • x64-osx
  • x64-linux

Does your PR follow the maintainer guide?

@luncliff luncliff marked this pull request as draft March 1, 2021 09:43
@luncliff
Copy link
Contributor Author

luncliff commented Mar 1, 2021

The port will be rebased when #16471 is merged. To prevent merge, it will remain in the draft mode.

@NancyLi1013 NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Mar 2, 2021
ports/tensorpipe/portfile.cmake Outdated Show resolved Hide resolved
ports/tensorpipe/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

Hi @luncliff

Thanks for your PR.

Have you tested these features?

@luncliff
Copy link
Contributor Author

luncliff commented Mar 2, 2021

@NancyLi1013 not yet. Doing so requires a Linux device with CUDA but I don't have one in mine. Asking my friends now :(

luncliff and others added 3 commits March 2, 2021 12:06
* it will be helpful to check output binaries are valid
* update code snapshot(2021/03/02) and port SHA
@luncliff
Copy link
Contributor Author

luncliff commented Mar 2, 2021

I'd like to try vcpkg_cmake_configure and vcpkg_cmake_install for this port. Any suggestions?

@NancyLi1013
Copy link
Contributor

I'd like to try vcpkg_cmake_configure and vcpkg_cmake_install for this port. Any suggestions?

Currently, there is no need to do any actions for the function changes. Just keep them as before. Maybe we will update them to new functions in the near future, but not now.

ports/tensorpipe/portfile.cmake Outdated Show resolved Hide resolved
ports/tensorpipe/fix-cmakelists.patch Outdated Show resolved Hide resolved
@luncliff luncliff marked this pull request as ready for review March 20, 2021 12:16
@NancyLi1013
Copy link
Contributor

Hi @luncliff

Could you please help confirm the test status of features?

@luncliff
Copy link
Contributor Author

luncliff commented Mar 22, 2021

Sadly, the tensorpipe_test keeps failing. (For those Linux features...)

  • pybind11: needs an additional patch for its build. I will update the fix-cmakelists.patch within days.
  • cma, ibv, cuda: build passed, but tensorpipe_test failes with infinite loop
  • shm, benchmark: seems like these are OK

Does every test executable in the port must report green? 🤔 If so, I will change this to draft again then check the mainstream's CI.

@NancyLi1013
Copy link
Contributor

Generally, we should keep all features work normally for every port. If one or more of these features cannot work. it would be better to fix them. If they can only support the specific platform, we should post some messages to describe it. Otherwise, you can choose to remove them from feature lists until they can work fine.

@NancyLi1013 NancyLi1013 added requires:testing Needs tests added before merging and removed requires:author-response labels Mar 24, 2021
@luncliff
Copy link
Contributor Author

luncliff commented Mar 24, 2021

I see. I'd better remove cma and ibv which are not passing the test. I want to support cuda, but it may not that easy to debug with current state.. :(
I will try more after fixing pybind11 related works.

@luncliff luncliff marked this pull request as draft March 24, 2021 06:58
* simplify features
* fix `pybind11` build failures
* updates source code base and CMake export file names
* use GNUInstallDirs variable
* use more correct "support" exporession
@luncliff luncliff mentioned this pull request Apr 10, 2021
19 tasks
@luncliff
Copy link
Contributor Author

luncliff commented May 1, 2021

After 97bb90b, tensorpipe's CUDA test passes.

user@host:~/vcpkg$ ./vcpkg install tensorpipe[cuda,test]
Computing installation plan...
The following packages will be built and installed:
    tensorpipe[core,cuda,test]:x64-linux -> 2021-04-26
Detecting compiler hash for triplet x64-linux...
Could not locate cached archive: /home/luncliff/.cache/vcpkg/archives/5f/5fbca65575504f45776aebee0cf1f263fa52ac5d.zip
Starting package 1/1: tensorpipe:x64-linux
Building package tensorpipe[core,cuda,test]:x64-linux...
-- Using cached /home/luncliff/vcpkg/downloads/pytorch-tensorpipe-c5a21994bc766659f7f85edb75478e13f429f46c.tar.gz
-- Cleaning sources at /home/luncliff/vcpkg/buildtrees/tensorpipe/src/13f429f46c-d111f7299b.clean. Use --editable to skip cleaning for the packages you specify.
-- Extracting source /home/luncliff/vcpkg/downloads/pytorch-tensorpipe-c5a21994bc766659f7f85edb75478e13f429f46c.tar.gz
-- Applying patch fix-cmakelists.patch
-- Applying patch support-test.patch
-- Applying patch support-pybind11.patch
-- Using source at /home/luncliff/vcpkg/buildtrees/tensorpipe/src/13f429f46c-d111f7299b.clean
...
Installing package tensorpipe[core,cuda,test]:x64-linux... done
Elapsed time for package tensorpipe:x64-linux: 1.606 min

Total elapsed time: 1.612 min

The package tensorpipe:x64-linux provides CMake targets:

    find_package(tensorpipe CONFIG REQUIRED)
    target_link_libraries(main PRIVATE tensorpipe)

The test programs are located in each of the build directories.

user@host:~/vcpkg$ pushd ./buildtrees/tensorpipe/x64-linux-dbg/tensorpipe/test/
~/vcpkg/buildtrees/tensorpipe/x64-linux-dbg/tensorpipe/test ~/vcpkg
user@host:~/vcpkg/buildtrees/tensorpipe/x64-linux-dbg/tensorpipe/test$ ./tensorpipe_test
...
[  SKIPPED ] CudaIpc/CudaMultiGPUChannelTestSuite.SendReverseAcrossDevices/0
[  SKIPPED ] CudaIpc/CudaMultiGPUChannelTestSuite.SendAcrossNonDefaultDevices/0

  YOU HAVE 4 DISABLED TESTS

@luncliff luncliff marked this pull request as ready for review May 1, 2021 12:16
@luncliff
Copy link
Contributor Author

luncliff commented May 1, 2021

For x64-osx failure, this will be rebased after #17610

@luncliff
Copy link
Contributor Author

luncliff commented May 3, 2021

Test execution in Darwin also passed

user@host$ pushd /Users/user/dev/vcpkg/buildtrees/tensorpipe/x64-osx-dbg/tensorpipe/test
user@host$ ./tensorpipe_test 
...
[----------] 1 test from Mpt/MptChannelTestSuite
[ RUN      ] Mpt/MptChannelTestSuite.ContextIsNotJoined/0
[       OK ] Mpt/MptChannelTestSuite.ContextIsNotJoined/0 (1 ms)
[----------] 1 test from Mpt/MptChannelTestSuite (1 ms total)

[----------] Global test environment tear-down
[==========] 52 tests from 15 test suites ran. (6289 ms total)
[  PASSED  ] 52 tests.

  YOU HAVE 2 DISABLED TESTS

@luncliff luncliff requested a review from NancyLi1013 May 6, 2021 02:03
@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:testing Needs tests added before merging labels May 7, 2021
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for adding this port @luncliff.

@dan-shaw dan-shaw merged commit c76ec6f into microsoft:master May 18, 2021
@luncliff luncliff deleted the port/tensorpipe branch May 18, 2021 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants