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-cuda12] new port #38275

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vipcxj
Copy link
Contributor

@vipcxj vipcxj commented Apr 19, 2024

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

There is already a port named onnxruntime-gpu. But it use cuda 11. And cuda 11 is still the default cuda version of onnxruntime-gpu. This port introduce the onnxruntime-gpu with cuda 12.

@vipcxj
Copy link
Contributor Author

vipcxj commented Apr 20, 2024

I'd like to upgrade onnxruntime-gpu if needed, but the cuda I'm currently using is all 12, and having multiple versions of cuda on one machine could cause quite a few problems!

@FrankXie05 FrankXie05 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 22, 2024
@vipcxj
Copy link
Contributor Author

vipcxj commented Apr 28, 2024

@FrankXie05 Any news?

@FrankXie05 FrankXie05 added the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Apr 28, 2024
@FrankXie05
Copy link
Contributor

All features and usage have been tested successfully on x64-windows.

@FrankXie05 FrankXie05 added info:reviewed Pull Request changes follow basic guidelines and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Apr 29, 2024
@data-queue
Copy link
Contributor

I understand why you are trying to make this change, but this should be an overlay port or in a custom registry for now. I think the port should use the default version of CUDA recommended by upstream. There is cost for maintaining multiple versions of a port (opencv2, opencv3, opencv4), and we shouldn't really be doing that in general.

@data-queue data-queue marked this pull request as draft May 2, 2024 02:53
@JoergAtGithub
Copy link
Contributor

There is also #36850 which uses port features for the different execution providers.

@vipcxj
Copy link
Contributor Author

vipcxj commented May 2, 2024

There is also #36850 which uses port features for the different execution providers.

There's no telling when this pr will be merged at all I guess, this pr is compiled with the source code to replace the current direct download version, it looks like it's going to be a long work in progress.

@vipcxj
Copy link
Contributor Author

vipcxj commented May 2, 2024

I understand why you are trying to make this change, but this should be an overlay port or in a custom registry for now. I think the port should use the default version of CUDA recommended by upstream. There is cost for maintaining multiple versions of a port (opencv2, opencv3, opencv4), and we shouldn't really be doing that in general.

Like @JoergAtGithub mentioned, if that pr is finished, I won't need further maintenance on this port, but isn't he currently not finished? I don't know when it will be finished either.

@vipcxj vipcxj marked this pull request as ready for review May 2, 2024 10:49
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This port conflicts with an existing port:

error: The following files are already installed in D:/vcpkg/installed/x64-windows and are in conflict with onnxruntime-gpu:x64-windows
Installed by onnxruntime-cuda12:x64-windows
bin/onnxruntime.dll
    bin/onnxruntime.pdb
    bin/onnxruntime_providers_cuda.dll
    bin/onnxruntime_providers_cuda.pdb
    bin/onnxruntime_providers_shared.dll
    bin/onnxruntime_providers_shared.pdb
    bin/onnxruntime_providers_tensorrt.dll
    bin/onnxruntime_providers_tensorrt.pdb
    debug/bin/onnxruntime.dll
    debug/bin/onnxruntime.pdb
    debug/bin/onnxruntime_providers_cuda.dll
    debug/bin/onnxruntime_providers_cuda.pdb
    debug/bin/onnxruntime_providers_shared.dll
    debug/bin/onnxruntime_providers_shared.pdb
    debug/bin/onnxruntime_providers_tensorrt.dll
    debug/bin/onnxruntime_providers_tensorrt.pdb
    debug/lib/onnxruntime.lib
    debug/lib/onnxruntime_providers_cuda.lib
    debug/lib/onnxruntime_providers_shared.lib
    debug/lib/onnxruntime_providers_tensorrt.lib
    lib/onnxruntime.lib
    lib/onnxruntime_providers_cuda.lib
    lib/onnxruntime_providers_shared.lib
    lib/onnxruntime_providers_tensorrt.lib
@BillyONeal BillyONeal changed the title [onnxruntim-cuda12] new port May 2, 2024
@vipcxj
Copy link
Contributor Author

vipcxj commented May 3, 2024

@BillyONeal It will definitely conflict with onnxruntime-gpu because this port is the onnxruntime which using the cuda12, using the same dll or so with onnxruntime-gpu, do you have any suggestions for modifications? In fact if a future compiled version of onnxruntime is merged, it's bound to conflict with onnxruntime-gpu as well. Normally you wouldn't install more than one version of onnxruntime at the same time, because even if you avoid the conflict by modifying the name of the dll or so, all the functions are still the same and still in conflict, you can't link different versions of onnxruntime at the same time!

@data-queue
Copy link
Contributor

There's an open question on how to represent onnxruntime and its providers in a vcpkg port:

  1. Creating a port for each onnxruntime provider and version would be too expensive to maintain long term for vcpkg.
  2. Having features to represent each provider (features as alternatives) would go against our model of how features work.

Some potential options to improve the situation:

  1. Add documentation in the port to recommend users to create an overlay port or maintain this in a custom registry.
  2. Change the default installation for each platform.
  3. Maybe there are other ideas?

Longer term, we may revisit our features as alternatives policy to account for scenarios like this.

@data-queue data-queue marked this pull request as draft May 6, 2024 22:24
@vipcxj
Copy link
Contributor Author

vipcxj commented May 7, 2024

There's an open question on how to represent onnxruntime and its providers in a vcpkg port:

  1. Creating a port for each onnxruntime provider and version would be too expensive to maintain long term for vcpkg.
  2. Having features to represent each provider (features as alternatives) would go against our model of how features work.

Some potential options to improve the situation:

  1. Add documentation in the port to recommend users to create an overlay port or maintain this in a custom registry.
  2. Change the default installation for each platform.
  3. Maybe there are other ideas?

Longer term, we may revisit our features as alternatives policy to account for scenarios like this.

Creating an overlay port is not a option. You can't expect everyone to know how to write portfile, even if it's just downloading a precompiled link library that doesn't involve compilation. And in order for cmake to find it, users need to have a deep understanding of the mechanics of cmake as well. Isn't open source about not building the wheel over and over again?

@data-queue
Copy link
Contributor

Hi @vipcxj unfortunately, we can't accept this port into the main catalog until we figure out a better solution for our features as alternatives policy. There are changes in this PR, such as the CMake config changes, that we can accept into the onnxruntime-gpu port if you are interested in making that PR.

@BillyONeal
Copy link
Member

BillyONeal commented Jun 10, 2024

Creating an overlay port is not a option. You can't expect everyone to know how to write portfile, even if it's just downloading a precompiled link library that doesn't involve compilation. And in order for cmake to find it, users need to have a deep understanding of the mechanics of cmake as well. Isn't open source about not building the wheel over and over again?

I'm sorry that there was some confusion over this. You can write instructions in the 'onnxruntime' port that says 'to get the CUDA12 one, copy this as an overlay port and make these changes'. We are not going to be able to accept ports like this that conflict with each other for the foreseeable future. Before registries were a feature, we accepted some alternatives, such as boringssl, into the registry. However, now that registries are a feature, we have refrained from accepting anything else like this as being in the curated registry is a statement that we have tested the component together with other parts of the curated registry, which we are unable to do in these alternatives cases.

See also https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#unique-port-attribution-rule
See also https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#recommended-solutions

Example ports already following this pattern include:

  • casclib
  • glad
  • libgit2
  • libgme
@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Jun 10, 2024
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!
5 participants