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.

Sign up for GitHub

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: enable cuda provider #20392

Closed
wants to merge 5 commits into from
Closed

onnxruntime: enable cuda provider #20392

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0e355cd
onnxruntime: enable cuda provider
bverhagen Oct 6, 2023
b09d0e7
onnxruntime: patch 1.16.0 build too
bverhagen Oct 9, 2023
8416420
onnxruntime: patch 1.16.1 build too
bverhagen Oct 13, 2023
5cb32e7
onnxruntime: avoid gsl being patched when cuda is active
bverhagen Oct 17, 2023
cf8914e
Merge branch 'master' into master
bverhagen Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions recipes/onnxruntime/all/conandata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,29 @@ patches:
- patch_file: "patches/1.14.1-0004-abseil-no-string-view.patch"
patch_description: "allow to build with abseil built without c++17 support"
patch_type: "portability"
- patch_file: "patches/1.16.1-0002-cuda-gsl.patch"
patch_description: "CMake: fix GSL and cuda"
patch_type: "conan"
"1.16.0":
- patch_file: "patches/1.16.0-0001-cmake-dependencies.patch"
patch_description: "CMake: ensure conan dependencies are used"
patch_type: "conan"
- patch_file: "patches/1.14.1-0004-abseil-no-string-view.patch"
patch_description: "allow to build with abseil built without c++17 support"
patch_type: "portability"
- patch_file: "patches/1.16.0-0002-cuda-gsl.patch"
patch_description: "CMake: fix GSL and cuda"
patch_type: "conan"
"1.15.1":
- patch_file: "patches/1.15.1-0001-cmake-dependencies.patch"
patch_description: "CMake: ensure conan dependencies are used"
patch_type: "conan"
- patch_file: "patches/1.14.1-0004-abseil-no-string-view.patch"
patch_description: "allow to build with abseil built without c++17 support"
patch_type: "portability"
- patch_file: "patches/1.15.1-0002-cuda-gsl.patch"
patch_description: "CMake: fix GSL and cuda"
patch_type: "conan"
"1.14.1":
- patch_file: "patches/1.14.1-0001-cmake-dependencies.patch"
patch_description: "CMake: ensure conan dependencies are used (upstreamed future versions)"
Expand All @@ -52,3 +61,6 @@ patches:
patch_description: "Ensures the forward compatibility with the newest versions of re2 library."
patch_type: "portability"
patch_source: "https://github.com/microsoft/onnxruntime/commit/126e7bf15fa4af8621814b82a3f7bd0d786f0239.patch"
- patch_file: "patches/1.14.1-0006-cuda-gsl.patch"
patch_description: "CMake: fix GSL and cuda"
patch_type: "conan"
9 changes: 6 additions & 3 deletions recipes/onnxruntime/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ class OnnxRuntimeConan(ConanFile):
"shared": [True, False],
"fPIC": [True, False],
"with_xnnpack": [True, False],
"use_cuda": [True, False],
}
default_options = {
"shared": False,
"fPIC": True,
"with_xnnpack": False,
"use_cuda": False,
Copy link
Contributor

Choose a reason for hiding this comment

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

previous option is called with_XX, should we use the same naming across all the recipe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. I see that a bunch of other recipes use the option with_cuda. It would be good to change it to with_cuda so that it can easily be enabled across multiple recipes.

Copy link
Member

Choose a reason for hiding this comment

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

There is a recommendation for option names: https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/conanfile_attributes.md#recommended-names

}
short_paths = True

Expand Down Expand Up @@ -117,8 +119,8 @@ def validate_build(self):
)

def build_requirements(self):
# Required by upstream https://github.com/microsoft/onnxruntime/blob/v1.14.1/cmake/CMakeLists.txt#L5
self.tool_requires("cmake/[>=3.24 <4]")
# Required by upstream https://github.com/microsoft/onnxruntime/blob/v1.16.1/cmake/CMakeLists.txt#L5
self.tool_requires("cmake/[>=3.26 <4]")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a small oversight that should have been changed when adding 1.16.0. 1.14.1 is compatible with 3.24. I assume this is good enough and no disambiguation is required?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's probably fine to just raise the minimum.


def source(self):
get(self, **self.conan_data["sources"][self.version], strip_root=True)
Expand All @@ -135,6 +137,7 @@ def generate(self):
tc.variables["onnxruntime_USE_FULL_PROTOBUF"] = not self.dependencies["protobuf"].options.lite
tc.variables["onnxruntime_USE_XNNPACK"] = self.options.with_xnnpack

tc.variables["onnxruntime_USE_CUDA"] = self.options.use_cuda
tc.variables["onnxruntime_BUILD_UNIT_TESTS"] = False
tc.variables["onnxruntime_RUN_ONNX_TESTS"] = False
tc.variables["onnxruntime_GENERATE_TEST_REPORTS"] = False
Expand All @@ -159,7 +162,7 @@ def generate(self):
tc.variables["onnxruntime_TVM_CUDA_RUNTIME"] = False
tc.variables["onnxruntime_TVM_USE_HASH"] = False
tc.variables["onnxruntime_CROSS_COMPILING"] = False
tc.variables["onnxruntime_DISABLE_CONTRIB_OPS"] = False
tc.variables["onnxruntime_DISABLE_CONTRIB_OPS"] = self.options.use_cuda
tc.variables["onnxruntime_DISABLE_ML_OPS"] = False
tc.variables["onnxruntime_DISABLE_RTTI"] = False
tc.variables["onnxruntime_DISABLE_EXCEPTIONS"] = False
Expand Down
13 changes: 13 additions & 0 deletions recipes/onnxruntime/all/patches/1.14.1-0006-cuda-gsl.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/cmake/external/onnxruntime_external_deps.cmake b/cmake/external/onnxruntime_external_deps.cmake
index 0c41945778..9bb5ffc63b 100644
--- a/cmake/external/onnxruntime_external_deps.cmake
+++ b/cmake/external/onnxruntime_external_deps.cmake
@@ -250,7 +250,7 @@ if(onnxruntime_USE_CUDA)
GSL
URL ${DEP_URL_microsoft_gsl}
URL_HASH SHA1=${DEP_SHA1_microsoft_gsl}
- PATCH_COMMAND ${Patch_EXECUTABLE} --binary --ignore-whitespace -p1 < ${PROJECT_SOURCE_DIR}/patches/gsl/1064.patch
+ FIND_PACKAGE_ARGS 4.0 NAMES Microsoft.GSL
)
else()
FetchContent_Declare(
13 changes: 13 additions & 0 deletions recipes/onnxruntime/all/patches/1.15.1-0002-cuda-gsl.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/cmake/external/onnxruntime_external_deps.cmake b/cmake/external/onnxruntime_external_deps.cmake
index 9effd1a2db..db30f7a0d4 100644
--- a/cmake/external/onnxruntime_external_deps.cmake
+++ b/cmake/external/onnxruntime_external_deps.cmake
@@ -287,7 +287,7 @@ if(onnxruntime_USE_CUDA)
GSL
URL ${DEP_URL_microsoft_gsl}
URL_HASH SHA1=${DEP_SHA1_microsoft_gsl}
- PATCH_COMMAND ${Patch_EXECUTABLE} --binary --ignore-whitespace -p1 < ${PROJECT_SOURCE_DIR}/patches/gsl/1064.patch
+ FIND_PACKAGE_ARGS 4.0 NAMES Microsoft.GSL
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you submit this patch upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See microsoft/onnxruntime#17843

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I'm starting to wonder if we can go even further upstream. Would it be possible for the GSL to be updated with this path to support CUDA? That would be the ideal place to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, I did not look at the specific patch onnxruntime applies. I do think it is up to onnxruntime to take this up and consider it out of scope for here. It would be a transparent change for this conan package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I'm concerned that the patch is applied to the GSL Conan package in the Conan cache. Modifying a package in the cache in this way could cause unexpected problems since the package won't correlate to its hash after the patch is applied.

Copy link
Contributor

@jwillikers jwillikers Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@bverhagen, looking into this, it appears that adding FIND_PACKAGE_ARGS opts in to using find_package to attempt to find the installed version. Refer to the section on FIND_PACKAGE_ARGS here. Since the package requires ms-gsl, this should find the Conan installed version. It would then end up patching it. I think this would need to be fixed upstream in Microsoft.GSL in order to use the ms-gsl Conan package when CUDA is enabled.

I recommend requesting that the ONNX developers upstream their patch to GSL. Ironically, they are both Microsoft projects... so I imagine this wouldn't be difficult if it is possible.

In the meantime, you could instead modify the recipe to only require ms-gsl when use_cuda is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per the docs you linked: FIND_PACKAGE_ARGS is trying to resolve the package from the system using find_package() with the arguments you give it. If it finds it, it uses it and does not call ExternalProject with the remainder of the options (including PATCH_COMMAND).

Hence when it finds the system or conan ms-gsl, it is not going to patch it but simply use it instead (I guess the patch is no longer necessary or conan sets the right things).

It would also be pretty insane for CMake trying to patch it, as find_package() is intended to find system packages, which are typically not writable. Even more insane it would be to rewrite these for the whole system.

The patch is actually already merged upstream (see microsoft/GSL#1064), but not included in 4.0.0.

Even though we don't seem to have problems using conan's ms-gsl 4.0.0, it seems advisable to apply the patch from onnxruntime until a new version of ms-gsl is out. I'll adapt the requirements and add a summary of the above as a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jwillikers : I tried changing the PR so it does not depend on Conan's ms-gsl when CUDA is enabled. However, I got trapped in a situation where FetchContent_Declare in onnxruntime cmake does not download gsl when called from Conan while it works perfectly when called directly from the CLI.

Since it is only a problem until the next GSL release, I decided to not dive in further but to go for the second best option and use Conan's ms-gsl in all cases. To resolve your worries, I'll delete the PATCH_COMMAND in the *-cuda-gsl.patch files. Is that a good enough solution for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the conan file, I assume this is the culprit:

tc.variables["FETCHCONTENT_FULLY_DISCONNECTED"] = True

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a follow-up PR #20619 that gets rid of the FetchContent calls entirely. Could you test that one with use_cuda=True, perhaps?

)
else()
FetchContent_Declare(
13 changes: 13 additions & 0 deletions recipes/onnxruntime/all/patches/1.16.0-0002-cuda-gsl.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/cmake/external/onnxruntime_external_deps.cmake b/cmake/external/onnxruntime_external_deps.cmake
index 8e412c7847..efd599e5b3 100644
--- a/cmake/external/onnxruntime_external_deps.cmake
+++ b/cmake/external/onnxruntime_external_deps.cmake
@@ -301,7 +301,7 @@ if(onnxruntime_USE_CUDA)
GSL
URL ${DEP_URL_microsoft_gsl}
URL_HASH SHA1=${DEP_SHA1_microsoft_gsl}
- PATCH_COMMAND ${Patch_EXECUTABLE} --binary --ignore-whitespace -p1 < ${PROJECT_SOURCE_DIR}/patches/gsl/1064.patch
+ FIND_PACKAGE_ARGS 4.0 NAMES Microsoft.GSL
)
else()
FetchContent_Declare(
13 changes: 13 additions & 0 deletions recipes/onnxruntime/all/patches/1.16.1-0002-cuda-gsl.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/cmake/external/onnxruntime_external_deps.cmake b/cmake/external/onnxruntime_external_deps.cmake
index 0e2482d7de..4210e0bc5c 100644
--- a/cmake/external/onnxruntime_external_deps.cmake
+++ b/cmake/external/onnxruntime_external_deps.cmake
@@ -301,7 +301,7 @@ if(onnxruntime_USE_CUDA)
GSL
URL ${DEP_URL_microsoft_gsl}
URL_HASH SHA1=${DEP_SHA1_microsoft_gsl}
- PATCH_COMMAND ${Patch_EXECUTABLE} --binary --ignore-whitespace -p1 < ${PROJECT_SOURCE_DIR}/patches/gsl/1064.patch
+ FIND_PACKAGE_ARGS 4.0 NAMES Microsoft.GSL
)
else()
FetchContent_Declare(