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] update to latest #23569

Merged
merged 8 commits into from
May 18, 2022
Merged

[tensorpipe] update to latest #23569

merged 8 commits into from
May 18, 2022

Conversation

luncliff
Copy link
Contributor

What does your PR fix?

#17199 requires find_package(tensorpipe CONFIG)
Previous work #16472 installed tensorpipeTargets.cmake files but not tensorpipeConfig.cmake.

This update renames those tensorpipeTargets.cmake files to tensorpipe-config.cmake so find_package can work as expected.

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

Supported triplets.

  • x64-linux
  • x64-osx

Does your PR follow the maintainer guide?

Yes.

@luncliff luncliff changed the title [tensorpipe] version update [tensorpipe] update to latest Mar 15, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Mar 15, 2022

Maybe it would be good to have a patch that can be upstreamed? (pytorch/tensorpipe#320).
All they need is a config file with an include statement.
(Renaming the exported file works only as long as there is no need to add some find_dependency.)

In addition, tensorpipe-config.cmake will also match find_package(TenSorpiOe) etc.
I would stick with the original style, i.e. provide tensorpipeConfig.cmake (case-sensitive).

@JonLiu1993 JonLiu1993 self-assigned this Mar 16, 2022
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Mar 16, 2022
@luncliff
Copy link
Contributor Author

Ah. you're right. I will make a PR for this.
For now, I'm not sure if uv related link option can be handled gracefully.

@JonLiu1993
Copy link
Member

@luncliff , Thanks for your pr, I agree with @dg0yt , provide tensorpipeConfig.cmake keep the original style is better.

@JackBoosY
Copy link
Contributor

Ping for response.

* rename existing patch files
* update versions JSON
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/tensorpipe/vcpkg.json

Valid values for the license field can be found in the documentation

@luncliff
Copy link
Contributor Author

Sorry for late. I couldn't work for libtorch on work recently. pytorch/tensorpipe#435 is submitted and now we download the patch from it.

@JackBoosY JackBoosY added depends:different-pr This PR or Issue depends on a PR which has been filed and removed requires:author-response labels Mar 28, 2022
@luncliff luncliff mentioned this pull request Apr 21, 2022
19 tasks
@talregev
Copy link
Contributor

@luncliff @JackBoosY @JonLiu1993
We are waiting for pytorch/tensorpipe#435?
Should we approve with the patch?

@JackBoosY JackBoosY added requires:author-response and removed depends:different-pr This PR or Issue depends on a PR which has been filed labels May 7, 2022
@JackBoosY JackBoosY added the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label May 9, 2022
@JonLiu1993
Copy link
Member

@luncliff ,I tested the feature locally with the command "./vcpkg install tensorpipe[*]:x64-linux" but failed :

-- Downloading https://patch-diff.githubusercontent.com/raw/pytorch/tensorpipe/pull/435.diff -> tensorpipe-pr-435.patch...
[DEBUG] Feature flag 'binarycaching' unset
[DEBUG] Feature flag 'manifests' = off
[DEBUG] Feature flag 'compilertracking' unset
[DEBUG] Feature flag 'registries' unset
[DEBUG] Feature flag 'versions' unset
[DEBUG] 55511: popen(curl --fail -L https://patch-diff.githubusercontent.com/raw/pytorch/tensorpipe/pull/435.diff --create-dirs --output /home/vlilywang/Jon/vcpkg/downloads/tensorpipe-pr-435.patch.55511.part 2>&1)
[DEBUG] 55511: cmd_execute_and_stream_data() returned 0 after   328276 us
Error: Failed to download from mirror set:
File does not have the expected hash:
             url : [ https://patch-diff.githubusercontent.com/raw/pytorch/tensorpipe/pull/435.diff ]
       File path : [ /home/vlilywang/Jon/vcpkg/downloads/tensorpipe-pr-435.patch.55511.part ]
   Expected hash : [ 149539467ddd39feb6e715bf483d67954338998cbbcfef65de5a85831af902165f9347cd097fa8e82b10b2b8dbc388fcfe42664eeaf5de1954ae885f129583ed ]
     Actual hash : [ 7bcf604a967da36b8af936f8b8ab87b442834024b0b2cb886811c15e80893be842fbee2667bbbc39886814ec9b2f4ed0c2527de51fdb7dc293b25cce515f5e4b ]


[DEBUG] /mnt/vss/_work/1/s/src/vcpkg/base/downloads.cpp(709):
[DEBUG] Time in subprocesses: 329111 us
[DEBUG] Time in parsing JSON: 1 us
[DEBUG] Time in JSON reader: 0 us
[DEBUG] Time in filesystem: 78 us
[DEBUG] Time in loading ports: 0 us
[DEBUG] Exiting after 329.8 ms (328796 us)

CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:84 (message):

      Failed to download file with error: 1

@JackBoosY
Copy link
Contributor

According to the official doc, nvml.h is provided by NVML, this components should be installed by installing GPU Deployment Kit.
I think our Linux machine doesn't install it,

This feature can be build successfully on your machine, so I think that's okay for us.

@JonLiu1993 JonLiu1993 removed the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label May 16, 2022
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label May 16, 2022

list(APPEND TP_TEST_LINK_LIBRARIES
tensorpipe
- uv::uv
Copy link
Member

Choose a reason for hiding this comment

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

Are there official libuv targets we should be providing instead now?

PATCHES
fix-cmakelists.patch
"${INSTALL_PACKAGE_CONFIG_PATCH}"
use-vcpkg.patch
support-test.patch
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of patches in support of tests we generally don't want to build in the first place, but this is preexsisting. :)

@@ -25,20 +31,23 @@ if("pybind11" IN_LIST FEATURES)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

I know this is preexisting, but FYI this is almost certainly wrong: that this depends on building a Python suggests that it's building a Python plugin and should be using the built one, rather than one from the system.

@BillyONeal BillyONeal added depends:vm-update PR contains changes to the VM provisioning scripts and removed info:reviewed Pull Request changes follow basic guidelines labels May 16, 2022
@BillyONeal
Copy link
Member

Thanks for the update! This port's [cuda] feature depends on cuda-nvml-dev-11-6 so I want to avoid merging it until we take a VM update, which should be sometime this week.

@BillyONeal
Copy link
Member

VM update is #24695 -- just waiting for whether we are going to move to a different Azure region...

@BillyONeal
Copy link
Member

@luncliff @JackBoosY @JonLiu1993 We are waiting for pytorch/tensorpipe#435? Should we approve with the patch?

Upstream has had 2 months to respond to this patch, and the patch is restricted to build system only changes, so I think it's OK.

BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request May 17, 2022
BillyONeal added a commit that referenced this pull request May 17, 2022
* Install Ubuntu nasm package rather than building from source; 2.14 which is available in Ubuntu 20.04 is sufficient for intel-ipsec.

* Add cmake and ninja.

* Update CUDA signing key.

* Update pwsh to 7.2.3

* Remove clean downloads step obsoleted since we ripped out pacman.

* Note that haskell-stack is for bond.

* Cherry pick from #23569 : add cuda-nvml-dev-11-6

* Update pools.

* Baseline updates

PASSING, REMOVE FROM FAIL LIST: mesa:x64-windows-static-md (C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt).

:)

```
REGRESSION: cppgraphqlgen:arm64-windows failed with BUILD_FAILED. If expected, add cppgraphqlgen:arm64-windows=fail to C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: cppgraphqlgen:x64-windows failed with BUILD_FAILED. If expected, add cppgraphqlgen:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: cppgraphqlgen:x64-windows-static failed with BUILD_FAILED. If expected, add cppgraphqlgen:x64-windows-static=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: cppgraphqlgen:x64-windows-static-md failed with BUILD_FAILED. If expected, add cppgraphqlgen:x64-windows-static-md=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: cppgraphqlgen:x86-windows failed with BUILD_FAILED. If expected, add cppgraphqlgen:x86-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
```

This is a compiler behavior change or bug in VS 2022 16.2:

```
C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1432~1.313\bin\Hostx64\x86\cl.exe   /TP -DGRAPHQL_DLLEXPORTS -DIMPL_GRAPHQLSERVICE_DLL -Dgraphqlservice_EXPORTS -ID:\buildtrees\cppgraphqlgen\src\v4.3.1-9d04ffd723.clean\src\..\include -ID:\buildtrees\cppgraphqlgen\src\v4.3.1-9d04ffd723.clean\src\..\PEGTL\include -external:ID:\installed\x86-windows\include -external:W0 /nologo /DWIN32 /D_WINDOWS /W3 /utf-8 /GR /EHsc /MP  /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1  -MDd /W4 /WX /permissive- -std:c++20 /showIncludes /Fosrc\CMakeFiles\graphqlservice.dir\GraphQLService.cpp.obj /Fdsrc\CMakeFiles\graphqlservice.dir\ /FS -c D:\buildtrees\cppgraphqlgen\src\v4.3.1-9d04ffd723.clean\src\GraphQLService.cpp
cl : Command line warning D9025 : overriding '/W3' with '/W4'
D:\installed\x86-windows\include\tao\pegtl/demangle.hpp(147): error C2338: static_assert failed: 'internal::dependent_true< T > && ( begin != std::string_view::npos )'
D:\buildtrees\cppgraphqlgen\src\v4.3.1-9d04ffd723.clean\include\graphqlservice/internal/SyntaxTree.h(120): note: see reference to function template instantiation 'std::string_view tao::graphqlpeg::demangle<graphql::peg::variable_value>(void) noexcept' being compiled
D:\buildtrees\cppgraphqlgen\src\v4.3.1-9d04ffd723.clean\include\graphqlservice/internal/SyntaxTree.h(53): note: see reference to function template instantiation 'std::string_view graphql::peg::ast_node::type_name<graphql::peg::variable_value>(void) noexcept' being compiled
D:\buildtrees\cppgraphqlgen\src\v4.3.1-9d04ffd723.clean\src\GraphQLService.cpp(310): note: see reference to function template instantiation 'bool graphql::peg::ast_node::is_type<graphql::peg::variable_value>(void) noexcept const' being compiled
ninja: build stopped: subcommand failed.
```

This is a compiler behavior change in 17.2. @wravery do you have comments?

```
REGRESSION: qtwebengine:x64-windows failed with BUILD_FAILED. If expected, add qtwebengine:x64-windows=fail to C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtwebengine:x64-windows failed with BUILD_FAILED. If expected, add qtwebengine:x64-windows=fail to C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtwebengine:x64-windows failed with BUILD_FAILED. If expected, add qtwebengine:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtwebengine:x64-windows failed with BUILD_FAILED. If expected, add qtwebengine:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtwebengine:x64-windows failed with BUILD_FAILED. If expected, add qtwebengine:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtwebengine:x64-windows failed with BUILD_FAILED. If expected, add qtwebengine:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtwebengine:x64-windows failed with BUILD_FAILED. If expected, add qtwebengine:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
```

qtwebengine doesn't report logs, but I'm pretty sure it's this ICE: https://developercommunity.visualstudio.com/t/Visual-Studio-2022-v1720-reports-fata/10039296 @Neumann-A

```
REGRESSION: rsocket:x64-windows failed with BUILD_FAILED. If expected, add rsocket:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: rsocket:x64-windows-static failed with BUILD_FAILED. If expected, add rsocket:x64-windows-static=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: rsocket:x64-windows-static-md failed with BUILD_FAILED. If expected, add rsocket:x64-windows-static-md=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
```

ICE :( https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1490389

Other changes:
* Removed qt5-webengine skips because they are now skipped by a "supports" clause.
* Removed ctp skips because asset caching exists.

* [stxxl] Guard definition of log2 for current MSVCs.
@BillyONeal BillyONeal removed the depends:vm-update PR contains changes to the VM provisioning scripts label May 18, 2022
@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label May 18, 2022
@JackBoosY
Copy link
Contributor

LGTM.

@BillyONeal BillyONeal merged commit b34ae2a into microsoft:master May 18, 2022
@BillyONeal
Copy link
Member

Thanks for reconfirming @JackBoosY :)

BillyONeal added a commit that referenced this pull request Jan 18, 2023
* [onnx] feature: foxi

* https://github.com/houseroad/foxi
    * install the project's copyright (MIT)
* pytorch requires `foxi_loader`

The CMake target will be renamed to `onnxifi_*` for convenience.

* [onnx] force onnx/onnx_proto static in Windows

Checked the protject's CI logs.  It turned out onnx/onnx_proto are ALWAYS static.
Specify it in CMakeLists.txt because vcpkg configures `BUILD_SHARED_LIBS=ON`
If the triplet requires it.

There are no `ONNXIFI_ENABLE_EXT=ON` case.
Removed the misused build options in portfile.

Add port feature `protobuf-lite` which is in build option.

* [onnx] support windows static triplets

* remove SHARED for `onnxifi_wrapper` and `onnxifi_dummy`

* [onnx] fix wrong LICENSE install

* [onnx] remove feature 'foxi'

* also remove redundant part in patch files

* [libtorch] rework patch files

* [libtorch] config fixup ATen, Torch

* use `link_libraries` to vcpkg installed folder
  * future work may use library names without `find_library`
* update versions JSON to use `version-semver`

* [libtorch] make shared only

* Caffe2 is exported when BUILD_SHARED_LIBS

* [libtorch] remove headers after install

* [libtorch] rewrite patches and feature options

* checked osx / linux build

* [libtorch] use eigen3 always

* Caffe2 eigen_utils.h requires it

* [libtorch] error if BLAS feature collision

* [libtorch] remove !static

* [libtorch] replace vcpkg_find_acquire_program

* let's see python3 from find_program supports

* Dependency python3

* [libtorch] migrate works from luncliff/vcpkg-registry

* Update target version and dependencies
* Removed unsupported features

* [libtorch] misc fix, update version, baseline

* fix merge confict for 'onnx'

* [libtorch] install pip packages

* typing-extensions, pyyaml

* [libtorch] turn off Metal options

* [onnx] revert 'onnx' changes

* [libtorch] refine patches

* [libtorch] link with foxi_loader

* removed wrong onnx related source changes

* [libtorch] update git-tree

* [libtorch] reduce patch size

* [libtorch] find numa and activate USE_NUMA

* Update ports/libtorch/portfile.cmake

Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>

* Update ports/libtorch/portfile.cmake

Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>

* Update ports/libtorch/portfile.cmake

Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>

* [libtorch] fix mistype and update version JSON

* Add double quotes

* version

* Fix support expression

* version

* [libtorch] update cpuinfo usage

* #23944
* update version

* [tensorpipe] fix linux install

* find_package(Tensorpipe) may fail because of case mismatch

* [tensorpipe] update versions JSON

* [libtorch] fix feature failures

* [libtorch] remove CUDA feature

* [libtorch] giveup 'fbgemm' feature

* [libtorch] use mpi, openmpi in Linux

* [libtorch] fix glog link error

* [tensorpipe] bump port version

* see #23569

* [libtorch] fix patch list

* [libtorch] use official libuv config

* see #24745

* Update ports/libtorch/portfile.cmake

Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>

* Update ports/libtorch/portfile.cmake

Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>

* update versions JSON

* revert unnecessary 'nnpack' changes

* Update ports/libtorch/portfile.cmake

Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>

* [libtorch] use vcpkg-get-python-packages

* [libtorch] provide path of python3

* Update ports/libtorch/portfile.cmake

Co-authored-by: Billy O'Neal <bion@microsoft.com>

* Fix version database.

* [libtorch] use openmpi in linux/osx

* [libtorch] update to v1.12.1

* [libtorch] find_program(python3, python)

* [libtorch] provide PYTHON_EXECUTABLE directly

* [xnnpack] update to 2022-02-17

* [xnnpack] use C11, C++11

* [libtorch] more patches, DISABLE_PARALLEL_CONFIGURE

* [libtorch] allow static torch_cpu build

* Revert "[libtorch] allow static torch_cpu build"

This reverts commit 5fd4ef0.

* [libtorch] find_package(BLAS)

* [libtorch] simplify Python3, NumPy option use

* [libtorch] fix install in Windows

* [libtorch] exclude torch_global_deps in Windows

* [libtorch] platform of nnpack feature

* [libtorch] fix MPI option in Windows

* [libtorch] fixing LNK1161

* [libtorch] fix some mistypes

* [libtorch] define NOMINMAX for c10

* [libtorch] disable vulkan feature in Windows

* ci.baseline.txt: allow libtorch failure

* fails with Visual Studio 2022 17.4.2
* requires 17.4.3+

* Enable testing port on Windows

* [caffe2] redirect to libtorch

* update baseline

Co-authored-by: JackBoosY <yuzaiyang@beyondsoft.com>
Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>
Co-authored-by: Billy Robert O'Neal <bion@microsoft.com>
Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>
Co-authored-by: Victor Romero <romerosanchezv@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants