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

[VitisAI] Refactor the VAIEP to use MSFT's standalone API #19058

Merged
merged 9 commits into from
Feb 1, 2024
Merged

[VitisAI] Refactor the VAIEP to use MSFT's standalone API #19058

merged 9 commits into from
Feb 1, 2024

Conversation

BoarQing
Copy link
Contributor

@BoarQing BoarQing commented Jan 9, 2024

Description

Refactor the VAIEP to use MSFT's standalone API

Motivation and Context

Vitis ONNX RT VAI should switch to using the standalone API for ONNX EPs in order to decouple the EP from onnxruntime.dll and the providers.dll. This will help to simplify customer deployment of applications and use cases that need to share their onnxruntime.dll with other applications.

@zz002
Copy link
Contributor

zz002 commented Jan 15, 2024

@jywu-msft Hi, could you start the pipeline?

@BoarQing
Copy link
Contributor Author

@jywu-msft Could you start the pipeline when avaliable? It would means a lot for VitisAI.

@jywu-msft
Copy link
Member

can you fix the python lint errors?

@jywu-msft
Copy link
Member

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline

Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@jywu-msft
Copy link
Member

there are build failures in the Linux CPU Minimal and MacOS builds

@jywu-msft
Copy link
Member

/azp run Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,Windows x64 QNN CI Pipeline, Linux OpenVINO CI Pipeline

Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@jywu-msft
Copy link
Member

/azp run Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,Windows x64 QNN CI Pipeline, Linux OpenVINO CI Pipeline

@jywu-msft
Copy link
Member

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline

Copy link

Azure Pipelines successfully started running 8 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@jywu-msft
Copy link
Member

jywu-msft commented Jan 24, 2024

Thanks for this PR. I see you made the changes so that vitis ai ep can be built as a shared library and decoupled from onnxruntime.dll
The only controversial change I see here is that the factory creation method will be available by default in all builds.
The current pattern is that an EP's create method is included when the build for that EP is enabled.
What is the use case for the change? How do you envision vitisai ep shared library will be built? Is the intention to provide vitis ep dll's that are compatible with the onnxruntime.dll from an official release package?
Adding a couple more people to this discussion as we're looking at enabling EP development outside the main onnxruntime repo and registration of the dll at runtime.
+@souptc , @pranavsharma

@zz002
Copy link
Contributor

zz002 commented Jan 24, 2024

The factory creation methods of other shared libraries are also available by default in all versions.
vitisai ep is closed source, so test cases cannot be provided. No release package is currently available.

@jywu-msft
Copy link
Member

The factory creation methods of other shared libraries are also available by default in all versions. vitisai ep is closed source, so test cases cannot be provided. No release package is currently available.

i'm wondering about these changes:

#if !defined(ORT_MINIMAL_BUILD)

@zz002
Copy link
Contributor

zz002 commented Jan 25, 2024

The factory creation methods of other shared libraries are also available by default in all versions. vitisai ep is closed source, so test cases cannot be provided. No release package is currently available.

i'm wondering about these changes:

#if !defined(ORT_MINIMAL_BUILD)

#if defined(ORT_MINIMAL_BUILD)

Similar to the above code, create_not_supported_status needs to be returned when ORT_MINIMAL_BUILD is defined. Because I have not customized provider_options, I put this code in the above function.
When defining ORT_MINIMAL_BUILD I need to create VitisAIProviderFactoryCreator, so the corresponding header file is included.

@jywu-msft
Copy link
Member

The factory creation methods of other shared libraries are also available by default in all versions. vitisai ep is closed source, so test cases cannot be provided. No release package is currently available.

i'm wondering about these changes:

#if !defined(ORT_MINIMAL_BUILD)

#if defined(ORT_MINIMAL_BUILD)

Similar to the above code, create_not_supported_status needs to be returned when ORT_MINIMAL_BUILD is defined. Because I have not customized provider_options, I put this code in the above function.
When defining ORT_MINIMAL_BUILD I need to create VitisAIProviderFactoryCreator, so the corresponding header file is included.

https://onnxruntime.ai/docs/build/custom.html#minimal-build
minimal builds are meant for supporting ORT format models, not ONNX models. as such, they are for not supported for most EP's.
That is why all those other api's for registering ep's/options are defined as stubs when

#if defined(ORT_MINIMAL_BUILD)

since you haven't implemented any such api's in provider_bridge, you don't need to worry about that case. there's no api's implemented in provider_bridge that you need to add a stub for a minimal build.

If you look at all the code above and below

#if !defined(ORT_MINIMAL_BUILD)
and

they all have if defined for their respective EP's, i think VitisAI should do the same. (not for all non-minimal builds)

@zz002
Copy link
Contributor

zz002 commented Jan 26, 2024

@jywu-msft I made some modifications, can you start the pipeline when available?

@jywu-msft
Copy link
Member

there are several options for supporting provider options.

if all your options can be represented by string keys and values, you can just use the generic api. i.e. you don't need to implement a VitisAI specific api.

ORT_API_STATUS_IMPL(SessionOptionsAppendExecutionProvider,
                    _In_ OrtSessionOptions* options,
                    _In_ const char* provider_name,
                    _In_reads_(num_keys) const char* const* provider_options_keys,
                    _In_reads_(num_keys) const char* const* provider_options_values,
                    _In_ size_t num_keys);

Otherwise, the other recommended pattern is using an opaque struct.
e.g. OrtTensorRTProviderOptionsV2

ORT_API_STATUS_IMPL(SessionOptionsAppendExecutionProvider_TensorRT_V2,
                    _In_ OrtSessionOptions* options, _In_ const OrtTensorRTProviderOptionsV2* tensorrt_options);
ORT_API_STATUS_IMPL(CreateTensorRTProviderOptions, _Outptr_ OrtTensorRTProviderOptionsV2** out);
ORT_API_STATUS_IMPL(UpdateTensorRTProviderOptions, _Inout_ OrtTensorRTProviderOptionsV2* tensorrt_options,
                    _In_reads_(num_keys) const char* const* provider_options_keys,
                    _In_reads_(num_keys) const char* const* provider_options_values,
                    size_t num_keys);
ORT_API_STATUS_IMPL(GetTensorRTProviderOptionsAsString, _In_ const OrtTensorRTProviderOptionsV2* tensorrt_options, _Inout_ OrtAllocator* allocator, _Outptr_ char** ptr);
ORT_API(void, ReleaseTensorRTProviderOptions, _Frees_ptr_opt_ OrtTensorRTProviderOptionsV2*);
ORT_API_STATUS_IMPL(UpdateTensorRTProviderOptionsWithValue, _Inout_ OrtTensorRTProviderOptionsV2* tensorrt_options, _In_ const char* key, _In_ void* value);
ORT_API_STATUS_IMPL(GetTensorRTProviderOptionsByName, _In_ const OrtTensorRTProviderOptionsV2* tensorrt_options, _In_ const char* key, _Outptr_ void** ptr);

you'll see there are api's to create the struct and update the struct.
The benefit of the opaque struct is that you can maintain abi compatibility across onnxruntime versions, since the struct is not directly accessed by the application linking against onnxruntime.dll/libonnxruntime.so

older api's such as

ORT_API_STATUS_IMPL(SessionOptionsAppendExecutionProvider_TensorRT,
                    _In_ OrtSessionOptions* options, _In_ const OrtTensorRTProviderOptions* tensorrt_options);

required application to directly update OrtTensorRTProviderOptions struct which does not guarantee abi compatibility.
that is why several EP's moved to the opaque v2 struct. (CUDA and TensorRT EP's are examples of those)

@zz002
Copy link
Contributor

zz002 commented Jan 26, 2024

The factory creation methods of other shared libraries are also available by default in all versions. vitisai ep is closed source, so test cases cannot be provided. No release package is currently available.

i'm wondering about these changes:

#if !defined(ORT_MINIMAL_BUILD)

#if defined(ORT_MINIMAL_BUILD)

Similar to the above code, create_not_supported_status needs to be returned when ORT_MINIMAL_BUILD is defined. Because I have not customized provider_options, I put this code in the above function.
When defining ORT_MINIMAL_BUILD I need to create VitisAIProviderFactoryCreator, so the corresponding header file is included.

https://onnxruntime.ai/docs/build/custom.html#minimal-build minimal builds are meant for supporting ORT format models, not ONNX models. as such, they are for not supported for most EP's. That is why all those other api's for registering ep's/options are defined as stubs when

#if defined(ORT_MINIMAL_BUILD)

since you haven't implemented any such api's in provider_bridge, you don't need to worry about that case. there's no api's implemented in provider_bridge that you need to add a stub for a minimal build.

If you look at all the code above and below

#if !defined(ORT_MINIMAL_BUILD)

and

they all have if defined for their respective EP's, i think VitisAI should do the same. (not for all non-minimal builds)

@jywu-msft
All shared libs use custom structures. Vitisai uses ProviderOptions, so it hopes to put the relevant processing in the function SessionOptionsAppendExecutionProvider. At the same time, since vitisai is a shared lib, it needs to provide VitisAIProviderFactoryCreator at any time when ORT_MINIMAL_BUILD is not defined. so the corresponding header file needs to be included.
Other eps are in this form because they are statically compiled, not shared lib.
I think the previous version of the code is more reasonable.If you agree with me, I will restore the code to the previous commit

@jywu-msft
Copy link
Member

jywu-msft commented Jan 26, 2024

The factory creation methods of other shared libraries are also available by default in all versions. vitisai ep is closed source, so test cases cannot be provided. No release package is currently available.

i'm wondering about these changes:

#if !defined(ORT_MINIMAL_BUILD)

#if defined(ORT_MINIMAL_BUILD)

Similar to the above code, create_not_supported_status needs to be returned when ORT_MINIMAL_BUILD is defined. Because I have not customized provider_options, I put this code in the above function.
When defining ORT_MINIMAL_BUILD I need to create VitisAIProviderFactoryCreator, so the corresponding header file is included.

https://onnxruntime.ai/docs/build/custom.html#minimal-build minimal builds are meant for supporting ORT format models, not ONNX models. as such, they are for not supported for most EP's. That is why all those other api's for registering ep's/options are defined as stubs when

#if defined(ORT_MINIMAL_BUILD)

since you haven't implemented any such api's in provider_bridge, you don't need to worry about that case. there's no api's implemented in provider_bridge that you need to add a stub for a minimal build.
If you look at all the code above and below

#if !defined(ORT_MINIMAL_BUILD)

and

they all have if defined for their respective EP's, i think VitisAI should do the same. (not for all non-minimal builds)

@jywu-msft All shared libs use custom structures. Vitisai uses ProviderOptions, so it hopes to put the relevant processing in the function SessionOptionsAppendExecutionProvider. At the same time, since vitisai is a shared lib, it needs to provide VitisAIProviderFactoryCreator at any time when ORT_MINIMAL_BUILD is not defined. so the corresponding header file needs to be included. Other eps are in this form because they are statically compiled, not shared lib. I think the previous version of the code is more reasonable.If you agree with me, I will restore the code to the previous commit

TensorRT EP, CUDA EP, OpenVINO EP and DNNL EP are all shared lib ep's and not statically compiled into onnxruntime.dll, so you can follow the same pattern they use.

@zz002
Copy link
Contributor

zz002 commented Jan 29, 2024

@jywu-msft I made some modifications, can you start the pipeline when available?

@jywu-msft
Copy link
Member

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline

Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@jywu-msft
Copy link
Member

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline

Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@jywu-msft
Copy link
Member

/azp run Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,Windows x64 QNN CI Pipeline, Linux OpenVINO CI Pipeline

Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@zz002
Copy link
Contributor

zz002 commented Feb 1, 2024

@jywu-msft Do you think there might be any other adjustments that could be considered?

@jywu-msft
Copy link
Member

@jywu-msft Do you think there might be any other adjustments that could be considered?

I will take another pass through the PR and get back to you.

@jywu-msft jywu-msft merged commit 1d6f13f into microsoft:main Feb 1, 2024
55 of 56 checks passed
jywu-msft pushed a commit that referenced this pull request Feb 2, 2024
### Description
<!-- Describe your changes. -->
Resolving compilation errors when using USE_VITISAI


### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->
There will be compilation errors when USE_VITISAI is enabled
This is in addition to the #19058

Co-authored-by: Zhenze Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants