-
Notifications
You must be signed in to change notification settings - Fork 3k
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/build] Add new flag enable_generic_interface to build primary EPs by default #23342
base: main
Are you sure you want to change the base?
Conversation
@karim-vad please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
ae0b626
to
713134d
Compare
… now a runtime error 'LoadLibrary failed with error 1114' when loading onnxruntime_providers_openvino.dll
when building onnxruntime.dll onnxruntime_shared.dll
by introducing EP specific Interface flags
Remove TODO[low] that are no longer applicable
with generic ORT interface
@@ -782,6 +782,12 @@ def convert_arg_line_to_args(self, arg_line): | |||
parser.add_argument("--use_triton_kernel", action="store_true", help="Use triton compiled kernels") | |||
parser.add_argument("--use_lock_free_queue", action="store_true", help="Use lock-free task queue for threadpool.") | |||
|
|||
parser.add_argument( | |||
"--enable_generic_interface", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a CI build that builds onnxruntime with this --enable_generic_interface
option (without explicitly enabling any EPs). This would help us ensure that the basic build works and would catch future regressions. Let me see if I can think of where we could add this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a new build stage to the existing Windows CPU CI Pipeline: https://github.com/microsoft/onnxruntime/blob/06fc73b7d4d80bd97e140776590d98b868c7bc3a/tools/ci_build/github/azure-pipelines/win-ci-pipeline.yml#L160C1-L179C1
Maybe something like:
- stage: x64_release_ep_generic_interface
dependsOn: []
jobs:
- template: templates/jobs/win-ci-vs-2022-job.yml
parameters:
BuildConfig: 'RelWithDebInfo'
buildArch: x64
additionalBuildFlags: --enable_generic_interface
msbuildPlatform: x64
isX86: false
job_name_suffix: x64_release_ep_generic_interface
RunOnnxRuntimeTests: false # --enable_generic_interface does not build tests
EnablePython: false
isTraining: false
ORT_EP_NAME: CPU
GenerateDocumentation: false
WITH_CACHE: false
MachinePool: 'onnxruntime-Win-CPU-2022'
Do you think this is appropriate @snnn ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And maybe in a future PR, we could enable --enable_generic_interface
with each EP combination and run unit tests. That is:
- build with
--enable_generic_interface --use_tensorrt
and run unit tests - build with
--enable_generic_interface --use_qnn
and run unit tests - etc.
We would have to allow building unit tests with --enable_generic_interface
as long as only we're only building with one non-cpu EP.
Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good consideration.
Few Qs?
- Do we want to enable generic build testing as part of this PR ?
- What tools/ci_build/build.py build argument corresponds to ORT_EP_NAME: CPU (templates/jobs/win-ci-vs-2022-job.yml)?
- Currently generic build has tests disabled. I guess we would have to relax that restriction when EP ( cpu or non-cpu) is also built with interface.
9d1469e
to
9f52af7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can commit the suggested changes from lintrunner.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Description
Motivation and Context
Changes in the build system required to evolve the repo to build the components independently while removing unnecessary dependencies