onnxruntime: enable cuda provider #20392

bverhagen · 2023-10-06T13:54:17Z

Specify library name and version: onnxruntime/1.15.1 and onnxruntime/1.14.1

We needed the GPU provider of onnxruntime, hence we share our changes to enable this for this conan package upstream.

We do not need features from contrib and since it does not build, it is disabled. Others who need it, can enable it in a next incremental step.

I've read the contributing guidelines.
I've used a recent Conan client version close to the currently deployed.
I've tried at least one configuration locally with the conan-center hook activated.

CLAassistant · 2023-10-06T13:54:24Z

All committers have signed the CLA.

recipes/onnxruntime/all/conanfile.py

jwillikers · 2023-10-06T16:39:38Z

recipes/onnxruntime/all/patches/1.15.1-0002-cuda-gsl.patch

+     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


Can you submit this patch upstream?

See microsoft/onnxruntime#17843

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.

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.

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.

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

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.

@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?

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

tc.variables["FETCHCONTENT_FULLY_DISCONNECTED"] = True

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?

bverhagen · 2023-10-09T07:39:01Z

Hi reviewers, I rebased (rather than merged) master to get the new 1.16.0 recipe in too. Hence I had to force push it. Apologies for making the review harder this way. Since these are not too many changes, I hope this is not too much of an inconvenience.

AbrilRBS · 2023-10-11T11:23:08Z

Apologies for making the review harder this way. Since these are not too many changes, I hope this is not too much of an inconvenience.

No worries :)

bverhagen · 2023-10-13T07:58:28Z

I tested and patched onnxruntime 1.16.1 too.

conan-center-bot · 2023-10-13T12:40:57Z

Conan v1 pipeline ✔️

All green in build 10 (8416420aa3eb76a0c10430355d649dba216ab96e):

onnxruntime/1.16.0:
All packages built successfully! (All logs)
onnxruntime/1.15.1:
All packages built successfully! (All logs)
onnxruntime/1.14.1:
All packages built successfully! (All logs)
onnxruntime/1.16.1:
All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 10 (8416420aa3eb76a0c10430355d649dba216ab96e):

onnxruntime/1.16.0:
All packages built successfully! (All logs)
onnxruntime/1.15.1:
All packages built successfully! (All logs)
onnxruntime/1.14.1:
All packages built successfully! (All logs)
onnxruntime/1.16.1:
All packages built successfully! (All logs)

jwillikers · 2023-10-16T12:34:16Z

recipes/onnxruntime/all/patches/1.15.1-0002-cuda-gsl.patch

+     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


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

bverhagen · 2023-10-17T09:45:15Z

recipes/onnxruntime/all/conanfile.py

-        # 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]")


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?

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

jwillikers · 2023-10-17T13:48:30Z

recipes/onnxruntime/all/conanfile.py

-        # 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]")


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

ilya-lavrenov · 2023-10-28T10:19:45Z

recipes/onnxruntime/all/conanfile.py

    }
    default_options = {
        "shared": False,
        "fPIC": True,
        "with_xnnpack": False,
+        "use_cuda": False,


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

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.

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

…ing cuda (#17843) ### Description Some CMake scripts reference Microsoft.GSL::GSL. Most of the time, the GSL package that is found on the system is used. However, when cuda is enabled, it is downloaded and patched. Most CMake scripts rely on the first case and forget about the second. This patch makes the second case behave like the first case. ### Motivation and Context This is an issue that occurs 'in the wild'. For example, I had to patch this to be able to enable the CUDA provider for the onnxruntime conan package (see conan-io/conan-center-index#20392).

stale · 2023-12-15T07:06:37Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

…ing cuda (microsoft#17843) ### Description Some CMake scripts reference Microsoft.GSL::GSL. Most of the time, the GSL package that is found on the system is used. However, when cuda is enabled, it is downloaded and patched. Most CMake scripts rely on the first case and forget about the second. This patch makes the second case behave like the first case. ### Motivation and Context This is an issue that occurs 'in the wild'. For example, I had to patch this to be able to enable the CUDA provider for the onnxruntime conan package (see conan-io/conan-center-index#20392).

czoido · 2024-09-04T14:27:26Z

Closed by #22557

bverhagen force-pushed the master branch from b25f9ab to 50dc747 Compare October 6, 2023 13:56

This was referenced Oct 6, 2023

[service] Potentially conflicting recipes #3571

Closed

onnxruntime: add version 1.16.0 #20076

Merged

prince-chrismc mentioned this pull request Oct 6, 2023

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

jwillikers suggested changes Oct 6, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

bverhagen mentioned this pull request Oct 9, 2023

fix reference to Microsoft.GSL::GSL in CMake build scripts when enabling cuda microsoft/onnxruntime#17843

Merged

bverhagen force-pushed the master branch from 7edcd38 to bd84bdc Compare October 9, 2023 07:35

conan-center-bot added the Failed label Oct 9, 2023

This comment has been minimized.

Sign in to view

bverhagen force-pushed the master branch 2 times, most recently from e453c8e to 836299a Compare October 9, 2023 11:43

conan-center-bot added the Unexpected Error label Oct 9, 2023

This comment has been minimized.

Sign in to view

conan-center-bot removed Unexpected Error Failed labels Oct 10, 2023

This comment has been minimized.

Sign in to view

AbrilRBS self-assigned this Oct 11, 2023

ghost mentioned this pull request Oct 11, 2023

onnxruntime: add version 1.16.1 #20527

Merged

3 tasks

bverhagen added 2 commits October 13, 2023 09:33

onnxruntime: enable cuda provider

0e355cd

onnxruntime: patch 1.16.0 build too

b09d0e7

bverhagen force-pushed the master branch from fe94591 to 4df6716 Compare October 13, 2023 07:57

onnxruntime: patch 1.16.1 build too

8416420

bverhagen force-pushed the master branch from 4df6716 to 8416420 Compare October 13, 2023 08:01

bverhagen requested review from jwillikers and AbrilRBS October 16, 2023 12:13

jwillikers suggested changes Oct 16, 2023

View reviewed changes

jwillikers approved these changes Oct 16, 2023

View reviewed changes

onnxruntime: avoid gsl being patched when cuda is active

5cb32e7

bverhagen commented Oct 17, 2023

View reviewed changes

valgur mentioned this pull request Oct 17, 2023

onnxruntime: simplify patching #20619

Closed

Merge branch 'master' into master

cf8914e

jwillikers approved these changes Oct 17, 2023

View reviewed changes

ilya-lavrenov reviewed Oct 28, 2023

View reviewed changes

ghost mentioned this pull request Nov 6, 2023

onnxruntime: don't bring link dependency to boost on conanv1 #20955

Merged

3 tasks

ghost mentioned this pull request Nov 11, 2023

onnxruntime: add version 1.16.2 #21072

Merged

3 tasks

ghost mentioned this pull request Nov 27, 2023

onnxruntime: add version 1.16.3, bump dependencies #21424

Merged

3 tasks

ghost mentioned this pull request Dec 24, 2023

onnxruntime: bump wil #21881

Merged

3 tasks

ghost mentioned this pull request Jan 4, 2024

onnxruntime: ensure conan dependencies are used when versions are overriden #22155

Closed

3 tasks

ghost mentioned this pull request Jan 15, 2024

[fix] fix onnxruntime android build #22359

Merged

3 tasks

fdgStilla mentioned this pull request Jan 26, 2024

onnxruntime: add with_cuda option #22557

Merged

3 tasks

ghost mentioned this pull request Jan 26, 2024

onnxruntime: More compatibility patches for 1.16 #22563

Closed

3 tasks

ghost mentioned this pull request Apr 7, 2024

onnxruntime: add 1.17.3 & 1.18.1, remove older versions #23409

Merged

3 tasks

czoido assigned czoido and unassigned AbrilRBS Sep 2, 2024

czoido closed this Sep 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

onnxruntime: enable cuda provider #20392

onnxruntime: enable cuda provider #20392

bverhagen commented Oct 6, 2023 •

edited

Loading

CLAassistant commented Oct 6, 2023 •

edited

Loading

jwillikers Oct 6, 2023

bverhagen Oct 9, 2023

jwillikers Oct 9, 2023

bverhagen Oct 10, 2023

jwillikers Oct 10, 2023

jwillikers Oct 16, 2023 •

edited

Loading

bverhagen Oct 16, 2023

bverhagen Oct 17, 2023

bverhagen Oct 17, 2023

valgur Oct 17, 2023

This comment has been minimized.

bverhagen commented Oct 9, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

AbrilRBS commented Oct 11, 2023

bverhagen commented Oct 13, 2023

conan-center-bot commented Oct 13, 2023

jwillikers Oct 16, 2023 •

edited

Loading

bverhagen Oct 17, 2023

jwillikers Oct 17, 2023

jwillikers Oct 17, 2023

ilya-lavrenov Oct 28, 2023

jwillikers Oct 28, 2023

uilianries Oct 31, 2023

stale bot commented Dec 15, 2023

czoido commented Sep 4, 2024

onnxruntime: enable cuda provider #20392

onnxruntime: enable cuda provider #20392

Conversation

bverhagen commented Oct 6, 2023 • edited Loading

CLAassistant commented Oct 6, 2023 • edited Loading

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

jwillikers Oct 16, 2023 • edited Loading

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

This comment has been minimized.

bverhagen commented Oct 9, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

AbrilRBS commented Oct 11, 2023

bverhagen commented Oct 13, 2023

conan-center-bot commented Oct 13, 2023

Conan v1 pipeline ✔️

Conan v2 pipeline ✔️

jwillikers Oct 16, 2023 • edited Loading

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

stale bot commented Dec 15, 2023

czoido commented Sep 4, 2024

bverhagen commented Oct 6, 2023 •

edited

Loading

CLAassistant commented Oct 6, 2023 •

edited

Loading

jwillikers Oct 16, 2023 •

edited

Loading

jwillikers Oct 16, 2023 •

edited

Loading