Skip to content

[NV EP] fix EP context options #24545

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

Merged
merged 7 commits into from
May 1, 2025
Merged

Conversation

gedoensmax
Copy link
Contributor

Description

While cleaning up the options I missed the part in the provider bridge that translates session options to TRT options.
To better integrate with current IHV work I adopted the principle that QNN and OV use to pipe through session options. Since all this is string based magic it would be great to be access a general point of truth like EpContextModelGenerationOptions in the provider wrappedtypes.

struct EpContextModelGenerationOptions {

This is a fix on top of #24456 @ankan-ban and @chilo-ms to review.

@ankan-ban
Copy link
Contributor

looks good. Thanks, Max, for the fix and additional cleanup!

@chilo-ms
Copy link
Contributor

/azp run Linux QNN CI Pipeline, Win_TRT_Minimal_CUDA_Test_CI, Windows ARM64 QNN CI Pipeline, Windows GPU Doc Gen CI Pipeline, Windows x64 QNN CI Pipeline

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@chilo-ms
Copy link
Contributor

please address lintrunner failure

@chilo-ms
Copy link
Contributor

Since we haven't set up a pipeline for NV EP, i tested it locally and encountered following compile error.

...
D:\ort\onnxruntime\core\providers\nv_tensorrt_rtx\nv_provider_factory_creator.h(16,60): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [D:\ort\build\Windows\Debug\on
nxruntime_session.vcxproj]
(compiling source file '../../../onnxruntime/core/session/provider_registration.cc')

D:\ort\onnxruntime\core\providers\nv_tensorrt_rtx\nv_provider_factory_creator.h(16,80): error C2143: syntax error: missing ',' before '*' [D:\ort\build\Windows\Debug\onnxruntime_session.vcxproj]
(compiling source file '../../../onnxruntime/core/session/provider_registration.cc')

D:\ort\onnxruntime\core\session\provider_registration.cc(292,61): error C2665: 'onnxruntime::NvProviderFactoryCreator::Create': no overloaded function could convert all the argument types [D:\ort\build\Window
s\Debug\onnxruntime_session.vcxproj]
D:\ort\onnxruntime\core\providers\nv_tensorrt_rtx\nv_provider_factory_creator.h(15,53):
could be 'std::shared_ptronnxruntime::IExecutionProviderFactory onnxruntime::NvProviderFactoryCreator::Create(const onnxruntime::ProviderOptions &,const int)'
D:\ort\onnxruntime\core\session\provider_registration.cc(292,61):
'std::shared_ptronnxruntime::IExecutionProviderFactory onnxruntime::NvProviderFactoryCreator::Create(const onnxruntime::ProviderOptions &,const int)': cannot convert argument 2 from 'onnxruntime::
SessionOptions *' to 'const int'
D:\ort\onnxruntime\core\session\provider_registration.cc(292,86):
There is no context in which this conversion is possible
D:\ort\onnxruntime\core\session\provider_registration.cc(292,61):
while trying to match the argument list '(onnxruntime::ProviderOptions, onnxruntime::SessionOptions *)'
...

@chilo-ms
Copy link
Contributor

/azp run Linux QNN CI Pipeline, Win_TRT_Minimal_CUDA_Test_CI, Windows ARM64 QNN CI Pipeline, Windows GPU Doc Gen CI Pipeline, Windows x64 QNN CI Pipeline

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@chilo-ms
Copy link
Contributor

/azp run Test Linux CUDA x64 Release, Test Linux TensorRT x64 Release, web_Debug / build_onnxruntime_web, web_Release / build_onnxruntime_web

Copy link

No pipelines are associated with this pull request.

chilo-ms
chilo-ms previously approved these changes Apr 25, 2025
@snnn
Copy link
Member

snnn commented Apr 26, 2025

/azp run Windows ARM64 QNN CI Pipeline

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gedoensmax
Copy link
Contributor Author

@anujj noticed that during rebasing I accidentally deleted the profile shape parsing. Just reverted that.

@chilo-ms
Copy link
Contributor

/azp run Linux QNN CI Pipeline, Win_TRT_Minimal_CUDA_Test_CI, Windows ARM64 QNN CI Pipeline, Windows GPU Doc Gen CI Pipeline, Windows x64 QNN CI Pipeline

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@chilo-ms
Copy link
Contributor

/azp run Linux QNN CI Pipeline, Win_TRT_Minimal_CUDA_Test_CI, Windows ARM64 QNN CI Pipeline, Windows GPU Doc Gen CI Pipeline, Windows x64 QNN CI Pipeline

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@jywu-msft
Copy link
Member

the 2 conflicts are due to #24552
take the changes from there.

@gedoensmax gedoensmax force-pushed the fix_ep_context_nv branch from dfbc6d3 to ffa63b0 Compare May 1, 2025 09:31
@chilo-ms
Copy link
Contributor

chilo-ms commented May 1, 2025

/azp run Linux QNN CI Pipeline, Win_TRT_Minimal_CUDA_Test_CI, Windows ARM64 QNN CI Pipeline, Windows GPU Doc Gen CI Pipeline, Windows x64 QNN CI Pipeline

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@chilo-ms chilo-ms merged commit 716a88f into microsoft:main May 1, 2025
81 checks passed
vraspar pushed a commit that referenced this pull request May 2, 2025
### Description

While cleaning up the options I missed the part in the provider bridge
that translates session options to TRT options.
To better integrate with current IHV work I adopted the principle that
QNN and OV use to pipe through session options. Since all this is string
based magic it would be great to be access a general point of truth like
`EpContextModelGenerationOptions` in the provider wrappedtypes.

https://github.com/microsoft/onnxruntime/blob/6df620675290d97d7e406faf232b8b521333b6e8/onnxruntime/core/framework/session_options.h#L73

This is a fix on top of #24456 @ankan-ban and @chilo-ms to review.
jywu-msft pushed a commit that referenced this pull request May 3, 2025
### Description

Cherry pick the following into
[rel-1.22.0](https://github.com/microsoft/onnxruntime/tree/rel-1.22.0)

- (#24608)
- (#24545)

---------

Co-authored-by: Changming Sun <[email protected]>
Co-authored-by: Maximilian Müller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants