-
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
Improve reduced ops and types build #9908
Conversation
@@ -59,3 +59,7 @@ onnxruntime/python/version_info.py | |||
.envrc | |||
.psenvrc | |||
*.csproj.user | |||
# exclude generated reduced kernel registration and type control |
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.
Do we want to specifically ignore these by list all 6 files here?
// This will prevent, | ||
// 1. Accidental commit of the reduced kernel registration files | ||
// 2. If the required ops config has changed, user has to revert the changes to | ||
// the kernel registration files |
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.
how about including either the original file or the modified file from cmake?
could we copy the file to the build directory instead of within the source tree? that may support multiple builds better.
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 thought about and tried that also, that approach will be modifying the onnxruntime_providers.cmake a lot to remove and add new files to many source lists for cpu and cuda eps, also need some change in onnxruntime_unittests.cmake for training ops. This is hard to maintain and also require same amount of work if we want to add reduced kernel for new EPs.
The current impl is not ideal, but it is relatively simple to implement and maintain.
We can certainly improve this later
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.
ok. I assume that was a response to my first question.
how about the second:
could we copy the file to the build directory instead of within the source tree?
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.
Actually create ***_reduced.cc/inc
in build folder also requires a lot of changes to the onnxruntime_providers.cmake
I have a working version here
https://github.com/microsoft/onnxruntime/tree/gwang-msft/better_reduced_build_test
I still prefer the current simple impl despite its limitation
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.
We also need to be careful about it working with different styles of build. IIRC it's possible to run the reduction script standalone prior to running build.py or cmake directly, so we need to allow for that. If I ran the script, it updated something in the build dir, and then I did a 'clean' it's somewhat opaque that I just lost the reduction info and need to run the script again.
Would it be relatively simple for the cmake to remove either the original or the reduced file from the globbed list (e.g. onnxruntime_provider_srcs) based on whether it's a reduced build or not? that way we don't need the ifdefs. something like:
if (onnxruntime_REDUCED_OPS_BUILD)
list(REMOVE_ITEM onnxruntime_providers_srcs ${ONNXRUNTIME_ROOT}/core/providers/cpu/cpu_execution_provider.cc)
else()
list(REMOVE_ITEM onnxruntime_providers_srcs ${ONNXRUNTIME_ROOT}/core/providers/cpu/cpu_execution_provider_reduced_ops.cc)
endif()
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.
one idea:
for the files involved in reduced op types, we could define a root path variable in cmake, say ${ONNXRUNTIME_OP_REDUCTION_ROOT}
. when no op reduction is done, that is simply ${ONNXRUNTIME_ROOT}
(maybe we'd need a similar one for training). otherwise, it is a path to another location (I'd prefer one in the build directory) that the modified files are in. regardless of root, we could keep the root-relative parts of the path the same.
then we can refer to ${ONNXRUNTIME_OP_REDUCTION_ROOT}
in the cmake files, add it as an include directory, and keep any relative references in the source files unchanged (e.g., in op_kernel_type_control.h).
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.
We also need to be careful about it working with different styles of build. IIRC it's possible to run the reduction script standalone prior to running build.py or cmake directly, so we need to allow for that. If I ran the script, it updated something in the build dir, and then I did a 'clean' it's somewhat opaque that I just lost the reduction info and need to run the script again.
perhaps adding a (required?) --build_dir
option to the reduction script would make it clearer.
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.
If you look at my other branch
https://github.com/microsoft/onnxruntime/tree/gwang-msft/better_reduced_build_test
I added an optional output_path
, we can change this to mandatory
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 still prefer my current impl which is very simple, which is an improvement over what we have now and will work for most cases.
The only issues are that it generates the reduced_op code files in src folder and use ifdef
Generating reduced_op code files in build folder or other folder is a better idea but will need more consideration for many cases.
We can log a future work item to further improve 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.
Description: Improve reduced ops and types build
Motivation and Context