[onnx] update ONNXIFI build output and host dependencies #20112

luncliff · 2021-09-11T16:11:58Z

What does your PR fix?

1. Feature for ONNXIFI

~~Pytorch project is using ONNX with this project's source code. (foxi_loader)~~
~~After a survey, I found there are differences in ONNX's public headers.~~
~~Making it to a separate port requires overwrite of those files. ~~

~~For #17199, replace related source files before build.~~
~~Then install the project's LICENSE (MIT) file with ONNX copyright. ~~

~~By this way, CMake target originally named foxi_* will be built as onnxifi_*.~~

2. Remove Host dependencies (help/suggestion needed)

In #18073, I used python3 for host dependencies to ensure pybind11 and find_package(Python3 COMPONENTS Development).

#17199 uses NumPy (when find_package(Python3)).
Seems like it should be prepared not by port but from environment...

3. Removed `ONNXIFI_EXT` option usage

I had a wrong understanding of the ONNXIFI_EXT build option. The project doesn't specify it in its CI. Removed it in portfile.cmake

4. New feature `protobuf-lite` for build option `ONNX_USE_LITE_PROTO`

vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    FEATURES
        pybind11      BUILD_ONNX_PYTHON
        protobuf-lite ONNX_USE_LITE_PROTO
)

5. `onnxifi` related targets always generate STATIC library in Windows

Checked the project's CI. It doesn't provide BUILD_SHARED_LIBS, so it seems like onnx.lib and onnx_proto.lib are default output....

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

No changes in triplet support.

Does your PR follow the maintainer guide?

Checking it.

* 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.

luncliff · 2021-09-16T05:09:39Z

The new feature creats onnxifi.dll and some others. This requires the port to support dynamic build.
Finding a way to support both static/dynamic protobuf

JackBoosY · 2021-09-16T08:04:15Z

Please ping me if this PR is ready for review or you need some help.

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.

* remove SHARED for `onnxifi_wrapper` and `onnxifi_dummy`

ports/onnx/fix-cmakelists.patch

ports/onnx/fix-foxi-sources.patch

ports/onnx/portfile.cmake

luncliff · 2021-09-17T09:47:54Z

I just read the guideline again, and I made a wrong decision. Using foxi as a feature doesn't fit the guideline... I had better remove those part.

I will find another workaround for #17199.

* also remove redundant part in patch files

…/onnx

JackBoosY · 2021-09-27T07:06:09Z

Let me take a look at why dynamic build doesn't work.

JackBoosY · 2021-09-27T07:49:46Z

In the official readme:

If you are building ONNX from source, it is recommended that you also build Protobuf locally as a static library. The version distributed with conda-forge is a DLL, but ONNX expects it to be a static library. Building protobuf locally also lets you control the verison of protobuf.

Since vcpkg is currently unable to establish a cross-triplet dependency, I think it's good now.

ports/onnx/portfile.cmake

ports/onnx/fix-cmakelists.patch

ports/onnx/wrap-onnxifi-targets.patch

luncliff · 2021-09-28T04:46:55Z

Got it. I will update patch and check it soon.

* onnxifi targets will be static either

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated

git add -u && git commit
git checkout 9d892ff9270f2d2fe3b8e490471f0a7dfa78981e -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/o-/onnx.json b/versions/o-/onnx.json
index 47f5b67..6641b09 100644
--- a/versions/o-/onnx.json
+++ b/versions/o-/onnx.json
@@ -11,4 +11,4 @@
       "port-version": 0
     }
   ]
-}
\ No newline at end of file
+}

JackBoosY · 2021-09-29T09:38:47Z

ports/onnx/vcpkg.json

-      "dependencies": [
-        "pybind11"
-      ]
+      "description": "Build Python binaries"


Why delete the dependency pybind11?

This was necessary when I tried to use FimdPython3 with find_package pybind11. But doing that blocks search of NumPy component. Same as the PR note

That is, the port needs Python 3 pybind but our pybind11 port is for Python 2?

No. It was quite confusing to me. Let me explain with PR comments...

OK. commented below. #20112 (comment)

ports/onnx/fix-cmakelists.patch

BillyONeal · 2021-10-06T22:06:36Z

ports/onnx/vcpkg.json

@@ -24,11 +22,11 @@
    }
  ],
  "features": {
+    "protobuf-lite": {


Features can't "remove" functionality or control alternatives; this should probably be a feature named protobuf which does the full thing and is in the default-features set.

I see. But I think default-features: ["protobuf"] can give misunderstanding.
Some users may think onnx[core,pybind11] is valid and it won't use protobuf.

protobuf in pybind11.dependencies will look weird.
But this one looks weird either in my opinion....

{ "dependencies": [ "protobuf", "python3" ], "default-features": [ "protobuf" ], "features": { "protobuf": { "description": "Build with full protobuf library" }, "protobuf-lite": { "description": "Use lite protobuf instead of full" }, "pybind11": { "description": "Build Python binaries" } } }

Some users may think onnx[core,pybind11] is valid and it won't use protobuf.

I believe that is unavoidable.

protobuf in pybind11.dependencies will look weird.

You can add onnx[core,protobuf] as a dependency of the pybind11 feature to express dependencies between features.

Does using protobuf-lite remove the protobuf dependency? If so, the dependency should be moved to the protobuf feature.

I will add dependency to onnx[core,protobuf] for pybind11

By the way, protobuf-lite also requires protobuf library and ptoroc in its tool.

luncliff · 2021-10-08T14:09:19Z

Handling Python3 issues in portfile

It seems like we have 3 use-cases for PYTHON3.

Since CMake 3.12, find_package(PythonInterp) and find_package(PythonLibs) are deprecated.
The project is using Python3, so I replaced them with find_package(Python3)

Case 1. Python3 Interpreter

Most port that uses PYTHON_EXECUTABLE(==Python3_EXECUTABLE) are under this category.
The python interpreter is required to invoke their scripts for build, codegen, etc.
Currently, they are handling this by acquiring PYTHON3.

# portfile.cmake
vcpkg_find_acquire_program(PYTHON3)

vcpkg_cmake_configure(
    SOURCE_PATH "${SOURCE_PATH}"
    OPTIONS
        -DPython3_EXECUTABLE=${PYTHON3}
        # ...
)

# CMakeLists.txt
find_package(Python3 COMPONENTS Interpreter)

If there is a port which have to solve find_package(pybind11 CONFIG), we can use "pybind11" in dependencies.

Case 2. Python3 Development

Currently, in Windows environment, vcpkg_find_acquire_program(PYTHON3) setups python executable, but not Python header files.
So, if the CMakeLists.txt wants Development component, it will fail.

# CMakeLists.txt
find_package(Python3 REQUIRED COMPONENTS Development) # requires Python's header files

This case, we can install the port python3 and use the header files in it. This is what I tried with providing a hint Python3_ROOT_DIR.

# portfile.cmake
# vcpkg_find_acquire_program(PYTHON3)

vcpkg_cmake_configure(
    SOURCE_PATH "${SOURCE_PATH}"
    OPTIONS
        # Only python.exe in Windows. We can't search Development component
        # -DPython3_EXECUTABLE=${PYTHON3}

        # search header files in ${vcpkg_root}/installed/${target_triplet}/include/python3/...
        -DPython3_ROOT_DIR=${CURRENT_INSTALLED_DIR} 
)

But, Interpreter component is for script execution and Development is for build. So vcpkg.json must contain host dependency together to fulfill both case.

{
  "dependencies": [
    "python3",
    {
      "name": "python3",
      "host": true
    }
  ]
}

Here, we can still use "pybind11" together to solve find_package(pybind11).

Case 3. Python3 NumPy

This case requires pip install numpy. Sadly we don't have a port which provides numpy header files.
Even though we have one, we can't sure those header files are the only thing we need for build...

find_package(Python3 COMPONENTS NumPy)

The port python3 may work for this case. We can find_program and use something like ${PYTHON3} -m pip install ....

However, if a project requires install of python packages, it is highly possible that those python packages are from a virtual environment.
The project's build is designed to build over it. Python headers, pybind11, numpy, etc.

I think ONNX is for 3rd case ports. That is, a port using ONNX wouldn't like to mix port python3 and Python headers in its virtual environment...

If so, mixing python3, pybind11 from vcpkg and NumPy from the virtual environment is too confusing.
This is the story why I removed "pybind11" from "dependencies".

see microsoft/vcpkg#20112

BillyONeal · 2021-10-11T18:18:51Z

Tagging requires:discussion to resolve the python mess. :/

luncliff · 2021-10-26T14:47:01Z

I give up this PR.
If the Python-related decision can't be made, I will drop those and reorganize ONNXIFI related changes and make new PR for recent update

[onnx] feature: foxi

8ee205b

* 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.

luncliff mentioned this pull request Sep 11, 2021

[libtorch] create a new port #17199

Merged

19 tasks

github-actions bot approved these changes Sep 11, 2021

View reviewed changes

luncliff marked this pull request as draft September 12, 2021 14:16

JonLiu1993 assigned JackBoosY Sep 13, 2021

JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Sep 13, 2021

github-actions bot approved these changes Sep 16, 2021

View reviewed changes

[onnx] support windows static triplets

5114b48

* remove SHARED for `onnxifi_wrapper` and `onnxifi_dummy`

github-actions bot approved these changes Sep 16, 2021

View reviewed changes

[onnx] fix wrong LICENSE install

28cec17

github-actions bot approved these changes Sep 16, 2021

View reviewed changes

luncliff marked this pull request as ready for review September 16, 2021 14:14

JackBoosY suggested changes Sep 17, 2021

View reviewed changes

JackBoosY added the requires:author-response label Sep 17, 2021

[onnx] remove feature 'foxi'

f8ec3f5

* also remove redundant part in patch files

luncliff changed the title ~~[onnx] support ONNXIFI Facebook Extension and update host dependencies~~ [onnx] update ONNXIFI build output and host dependencies Sep 18, 2021

Merge branch 'master' of https://github.com/microsoft/vcpkg into port…

18d4965

…/onnx

github-actions bot approved these changes Sep 23, 2021

View reviewed changes

JackBoosY removed the requires:author-response label Sep 27, 2021

JackBoosY suggested changes Sep 27, 2021

View reviewed changes

ports/onnx/portfile.cmake Outdated Show resolved Hide resolved

ports/onnx/fix-cmakelists.patch Outdated Show resolved Hide resolved

ports/onnx/wrap-onnxifi-targets.patch Show resolved Hide resolved

JackBoosY added the requires:author-response label Sep 27, 2021

[onnx] back to ONLY_STATIC_LIBRARY

233b72e

* onnxifi targets will be static either

github-actions bot requested changes Sep 28, 2021

View reviewed changes

[onnx] fix auto-formatted version JSON

94525df

github-actions bot approved these changes Sep 28, 2021

View reviewed changes

JackBoosY suggested changes Sep 29, 2021

View reviewed changes

JackBoosY requested a review from BillyONeal September 29, 2021 09:50

BillyONeal reviewed Oct 6, 2021

View reviewed changes

BillyONeal added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 6, 2021

luncliff added a commit to luncliff/vcpkg-registry that referenced this pull request Oct 9, 2021

[onnx] move work from luncliff/vcpkg

a59b789

see microsoft/vcpkg#20112

luncliff added a commit to luncliff/vcpkg-registry that referenced this pull request Oct 10, 2021

[onnx] move work from luncliff/vcpkg

381f46c

see microsoft/vcpkg#20112

BillyONeal added the requires:discussion label Oct 11, 2021

luncliff closed this Oct 26, 2021

luncliff mentioned this pull request Oct 26, 2021

Support ports for libtorch(PyTorch) luncliff/vcpkg-registry#10

Merged

luncliff deleted the port/onnx branch October 26, 2021 15:20

luncliff mentioned this pull request Nov 14, 2021

[onnx] update version to 1.10.2 and change patches #21409

Merged

strega-nil-ms removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. requires:discussion labels Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[onnx] update ONNXIFI build output and host dependencies #20112

[onnx] update ONNXIFI build output and host dependencies #20112

luncliff commented Sep 11, 2021 •

edited

Loading

luncliff commented Sep 16, 2021

JackBoosY commented Sep 16, 2021

luncliff commented Sep 17, 2021 •

edited

Loading

JackBoosY commented Sep 27, 2021

JackBoosY commented Sep 27, 2021

luncliff commented Sep 28, 2021

github-actions bot left a comment

JackBoosY Sep 29, 2021

luncliff Sep 29, 2021

BillyONeal Oct 6, 2021

luncliff Oct 8, 2021

luncliff Oct 8, 2021

BillyONeal Oct 6, 2021

luncliff Oct 8, 2021

BillyONeal Oct 11, 2021

luncliff Oct 13, 2021

luncliff Oct 13, 2021

luncliff commented Oct 8, 2021 •

edited

Loading

BillyONeal commented Oct 11, 2021

luncliff commented Oct 26, 2021

[onnx] update ONNXIFI build output and host dependencies #20112

[onnx] update ONNXIFI build output and host dependencies #20112

Conversation

luncliff commented Sep 11, 2021 • edited Loading

What does your PR fix?

1. Feature for ONNXIFI

2. Remove Host dependencies (help/suggestion needed)

3. Removed ONNXIFI_EXT option usage

4. New feature protobuf-lite for build option ONNX_USE_LITE_PROTO

5. onnxifi related targets always generate STATIC library in Windows

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

Does your PR follow the maintainer guide?

luncliff commented Sep 16, 2021

JackBoosY commented Sep 16, 2021

luncliff commented Sep 17, 2021 • edited Loading

JackBoosY commented Sep 27, 2021

JackBoosY commented Sep 27, 2021

luncliff commented Sep 28, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luncliff commented Oct 8, 2021 • edited Loading

Handling Python3 issues in portfile

Case 1. Python3 Interpreter

Case 2. Python3 Development

Case 3. Python3 NumPy

BillyONeal commented Oct 11, 2021

luncliff commented Oct 26, 2021

luncliff commented Sep 11, 2021 •

edited

Loading

3. Removed `ONNXIFI_EXT` option usage

4. New feature `protobuf-lite` for build option `ONNX_USE_LITE_PROTO`

5. `onnxifi` related targets always generate STATIC library in Windows

luncliff commented Sep 17, 2021 •

edited

Loading

luncliff commented Oct 8, 2021 •

edited

Loading