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

[onnxruntime] Add new port #14903

Closed
wants to merge 18 commits into from
Closed

[onnxruntime] Add new port #14903

wants to merge 18 commits into from

Conversation

pash-msft
Copy link

@pash-msft pash-msft commented Dec 2, 2020

Describe the pull request
Add new port for OnnxRuntime:
https://github.com/microsoft/onnxruntime

  • What does your PR fix? Fixes [New Port Request] onnxruntime  #14257
    This is a new port
  • Which triplets are supported/not supported? Have you updated the CI baseline?
    Only supported triplets are x86-windows-static and x86-linux-static
  • Does your PR follow the maintainer guide?
    Yes

@ghost
Copy link

ghost commented Dec 2, 2020

CLA assistant check
All CLA requirements met.

ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Show resolved Hide resolved
ports/onnxruntime/vcpkg.json Outdated Show resolved Hide resolved
ports/onnxruntime/vcpkg.json Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added category:new-port The issue is requesting a new library to be added; consider making a PR! requires:author-response labels Dec 3, 2020
@NancyLi1013 NancyLi1013 changed the title Add port for onnxruntime [onnxruntime] Add new port Dec 3, 2020
@NancyLi1013
Copy link
Contributor

@pash-msft
Could you please sign CLA first?

@jthelin
Copy link

jthelin commented Dec 3, 2020

Could you please sign CLA first?

I am working with @pash-msft to get his Github account correctly linked with his FTE microsoft.com account in the internal systems, and make the CLA Bot happy.

Thank you @NancyLi1013 for proceeding with the code review for this change while we work through the processes to get all the correct GH account linkage and GH org membership settings sorted out in the systems.

@pash-msft
Copy link
Author

Could you please sign CLA first?

I am working with @pash-msft to get his Github account correctly linked with his FTE microsoft.com account in the internal systems, and make the CLA Bot happy.

Thank you @NancyLi1013 for proceeding with the code review for this change while we work through the processes to get all the correct GH account linkage and GH org membership settings sorted out in the systems.

Thanks Jorgen. I followed your suggestion. The PR has now met CLA requirement.

ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
@pash-msft pash-msft requested a review from NancyLi1013 December 8, 2020 21:22
@NancyLi1013
Copy link
Contributor

The failures on x64-linux:

Use gtest from submodule
CMake Error at /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:165 (message):
  Could NOT find PythonInterp: Found unsuitable version "2.7.17", but
  required is at least "3.4" (found /usr/bin/python)
Call Stack (most recent call first):
  /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:456 (_FPHSA_FAILURE_MESSAGE)
  /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPythonInterp.cmake:169 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  /agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:625 (_find_package)
  CMakeLists.txt:506 (find_package)

Could you please look into this and try to fix it?

ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
onnxruntime:x64-uwp=fail
onnxruntime:x64-windows=fail
onnxruntime:x64-windows-static=fail
onnxruntime:x86-windows=fail
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you set all triplets fail here? If not supports, we should add supports field to vckg.json.

Copy link
Author

Choose a reason for hiding this comment

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

I was getting CI errors for the above triplets, the suggested solution was to add expected failures here. If I use vcpkg_fail_port_install, how do I tell CI pipeline to ignore failure on these unsupported ARCH and Target. Thanks for your feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add supports": "windows & !x86 & !uwp & !static & !arm & !wasm32 to vcpkg.json and remove these codes in ci.baseline.txt.

Please refer to https://github.com/microsoft/vcpkg/blob/master/ports/onnxruntime-gpu/vcpkg.json#L6.

Copy link
Author

Choose a reason for hiding this comment

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

I am not able to find right values here. There are only two triplets to be supported:
x64-linux
x64-windows-static-md
Could you please suggest me changes for vcpkg.json and portfile.cmake? Looks like I have to keep it the way it is. Let me know. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about x64-windows and x64-windows-static? Are they not supported?

x64-windows-static-md is community triplet, which will not be tested on CI pipeline.

If so, currently this port only supports Linux. We can add supports: linux to vcpkg.json and vcpkg_fail_port_install(ON_TARGET "windows" "osx") to portfile.cmake.

Copy link
Author

Choose a reason for hiding this comment

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

We plan to support x64-windows and x64-windows-static. Currently, we don't use these builds in our application. This can be extended to support future platforms.
Our application will use x64-windows-static-md build configuration. So, we can not have vcpkg_fail_port_install(ON_TARGET "windows" "osx") in the portfile.cmake correct?

If you are OK , can we have it approved. Have addressed other feedbacks. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

If these triplets are currently not officially supported, we can add supports instead of adding them here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes. We plan to add support incrementally. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these codes here.

@NancyLi1013
Copy link
Contributor

The latest failures on linux:

There should be no empty directories in /mnt/vcpkg-ci/packages/onnxruntime_x64-linux
The following empty directories were found:

    /mnt/vcpkg-ci/packages/onnxruntime_x64-linux/include/onnxruntime/core/providers/rocm/math

If a directory should be populated but is not, this might indicate an error in the portfile.
If the directories are not needed and their creation cannot be disabled, use something like this in the portfile to remove them:

    file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/a/dir" "${CURRENT_PACKAGES_DIR}/some/other/dir")


Found 1 error(s). Please correct the portfile:

@pash-msft
Copy link
Author

The failures on x64-linux:

Use gtest from submodule
CMake Error at /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:165 (message):
  Could NOT find PythonInterp: Found unsuitable version "2.7.17", but
  required is at least "3.4" (found /usr/bin/python)
Call Stack (most recent call first):
  /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:456 (_FPHSA_FAILURE_MESSAGE)
  /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPythonInterp.cmake:169 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  /agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:625 (_find_package)
  CMakeLists.txt:506 (find_package)

Could you please look into this and try to fix it?

Fixed it. Thanks

@pash-msft pash-msft closed this Jan 8, 2021
@pash-msft pash-msft reopened this Jan 8, 2021
@pash-msft
Copy link
Author

The failures on x64-linux:

Use gtest from submodule
CMake Error at /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:165 (message):
  Could NOT find PythonInterp: Found unsuitable version "2.7.17", but
  required is at least "3.4" (found /usr/bin/python)
Call Stack (most recent call first):
  /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:456 (_FPHSA_FAILURE_MESSAGE)
  /mnt/vcpkg-ci/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/share/cmake-3.18/Modules/FindPythonInterp.cmake:169 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  /agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:625 (_find_package)
  CMakeLists.txt:506 (find_package)

Could you please look into this and try to fix it?

fixed

@pash-msft pash-msft closed this Jan 8, 2021
@NancyLi1013 NancyLi1013 requested a review from JackBoosY January 13, 2021 09:43
"name": "onnxruntime",
"version-string": "1.5.3",
"description": "ONNX Runtime is a cross-platform inferencing and training accelerator compatible with many popular ML/DNN frameworks, including PyTorch, TensorFlow/Keras, scikit-learn.",
"homepage": "https://github.com/microsoft/onnxruntime"
Copy link
Contributor

@JackBoosY JackBoosY Jan 13, 2021

Choose a reason for hiding this comment

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

Suggested change
"homepage": "https://github.com/microsoft/onnxruntime"
"homepage": "https://github.com/microsoft/onnxruntime",
"supports:" "x64 & linux"

Copy link
Author

Choose a reason for hiding this comment

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

Only two triplets, x64-windows-static-md and x64-linux are supported. Don't think above works for x64-windows-static-md , correct? Once we add support of more of supported triplets, this can written as you suggest.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it should be x64 & linux.

onnxruntime:x64-uwp=fail
onnxruntime:x64-windows=fail
onnxruntime:x64-windows-static=fail
onnxruntime:x86-windows=fail
Copy link
Contributor

Choose a reason for hiding this comment

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

If these triplets are currently not officially supported, we can add supports instead of adding them here.

ports/onnxruntime/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pash-msft
Copy link
Author

The latest failures on linux:

There should be no empty directories in /mnt/vcpkg-ci/packages/onnxruntime_x64-linux
The following empty directories were found:

    /mnt/vcpkg-ci/packages/onnxruntime_x64-linux/include/onnxruntime/core/providers/rocm/math

If a directory should be populated but is not, this might indicate an error in the portfile.
If the directories are not needed and their creation cannot be disabled, use something like this in the portfile to remove them:

    file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/a/dir" "${CURRENT_PACKAGES_DIR}/some/other/dir")


Found 1 error(s). Please correct the portfile:

This should be fixed now.

@pash-msft pash-msft closed this Mar 9, 2021
@pash-msft pash-msft reopened this Mar 9, 2021
@pash-msft
Copy link
Author

@gapopu can you please take a look at pipeline failures?

@gapopu
Copy link

gapopu commented Mar 10, 2021

@NancyLi1013 I don't understand the reason why x64-windows-static-md is failing. I was able to get it passed in my local setup. Can you help me understand what the issue is?

@NancyLi1013
Copy link
Contributor

The failures on x64-windows-static-md

-- Found PythonInterp: D:/downloads/tools/python/python-3.9.2-x64/python.exe (found version "3.9.2") 
-- Found PythonInterp: D:/downloads/tools/python/python-3.9.2-x64/python.exe (found suitable version "3.9.2", minimum required is "3.5") 
CMake Error at D:/downloads/tools/cmake-3.19.2-windows/cmake-3.19.2-win32-x86/share/cmake-3.19/Modules/FindPackageHandleStandardArgs.cmake:218 (message):
  Could NOT find PythonLibs (missing: PYTHON_LIBRARIES PYTHON_INCLUDE_DIRS)
  (Required is at least version "3.5")
Call Stack (most recent call first):
  D:/downloads/tools/cmake-3.19.2-windows/cmake-3.19.2-win32-x86/share/cmake-3.19/Modules/FindPackageHandleStandardArgs.cmake:582 (_FPHSA_FAILURE_MESSAGE)
  D:/downloads/tools/cmake-3.19.2-windows/cmake-3.19.2-win32-x86/share/cmake-3.19/Modules/FindPythonLibs.cmake:310 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  C:/a/1/s/scripts/buildsystems/vcpkg.cmake:856 (_find_package)
  CMakeLists.txt:495 (find_package)

You can get the log here https://dev.azure.com/vcpkg/public/_build/results?buildId=50349&view=artifacts&pathAsName=false&type=publishedArtifacts

SOURCE_PATH "${SOURCE_PATH}/cmake"
PREFER_NINJA
OPTIONS
-Donnxruntime_ENABLE_PYTHON=ON
Copy link
Member

Choose a reason for hiding this comment

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

onnxruntime python wheels are normally distributed through pypi. Is there a need to build it through vcpkg? If not, you may set it to OFF.

@NancyLi1013
Copy link
Contributor

@pash-msft

Is work still being done for this PR?

@snnn
Copy link
Member

snnn commented Apr 23, 2021

Is there a plan to add ONNX there?

@NancyLi1013
Copy link
Contributor

ONNX

You can open a new issue for ONNX if you would like to add it in vcpkg.
Thanks.

@NancyLi1013
Copy link
Contributor

Thanks for the PR; we're closing this for now since there's been no response. If you'd like to continue working on it, please reopen and ping us!

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] onnxruntime
7 participants