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

Improve reduced ops and types build #9908

Merged
merged 8 commits into from
Dec 7, 2021

Conversation

guoyu-wang
Copy link
Contributor

Description: Improve reduced ops and types build

Motivation and Context

  • The current reduced build will modify the kernel registration and type controls directly, this has 2 problems,
  1. May cause accidental commit of the modified kernel registration and type specification
  2. If user modifies the required types and ops after initial build, the modified kernel registration and type specification must be reverted, which is not obvious to users and often ignored and causing missing kernel in execution
  • Modify the reduced build to modify the kernel registration and type specification in copied files instead of the original files, and use compile definition to control which file to use, to prevent the issues listed above
  • Fixed small test issue in Reduced CI

@@ -59,3 +59,7 @@ onnxruntime/python/version_info.py
.envrc
.psenvrc
*.csproj.user
# exclude generated reduced kernel registration and type control
Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

@guoyu-wang guoyu-wang Dec 3, 2021

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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()

Copy link
Contributor

@edgchen1 edgchen1 Dec 3, 2021

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).

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

skottmckay
skottmckay previously approved these changes Dec 7, 2021
Copy link
Contributor

@skottmckay skottmckay left a comment

Choose a reason for hiding this comment

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

:shipit:

@guoyu-wang guoyu-wang merged commit b34b991 into master Dec 7, 2021
@guoyu-wang guoyu-wang deleted the gwang-msft/better_reduced_build branch December 7, 2021 21:02
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