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

Make session configuration options available to kernels via OpKernelInfo #18897

Merged
merged 8 commits into from
Jan 13, 2024

Conversation

skottmckay
Copy link
Contributor

@skottmckay skottmckay commented Dec 20, 2023

Description

Pass through the ConfigOptions from the session via OpKernelInfo so that kernel behavior can be configured.

Initial usage would be to optionally enable a fast path for ARM64 bloat16 GEMM - see #17031
Other usages could be things like selected the exact implementations of the activation functions for RNN operators instead of the default approximations (e.g. use sigmoid_exact instead of sigmoid)

At least one alternative would be to add the ConfigOptions to the execution provider interface

  • Kernel could access via OpKernelInfo::GetExecutionProvider()
  • Would need to add to the base IExecutionProvider class for the config options to be available to all kernels
  • There would be a delay in adding the ConfigOptions to the EP instance
    • otherwise all EP creation functions need updating which is not practical
    • best place to add ConfigOptions to the EPs would be InferenceSession::Initialize as the config options aren't final until that point
    • this delay adds complexity for an EP author to understand when IExecutionProvider::ConfigOptions would be available for use. doesn't affect a kernel author though.
  • The configuration options are not really an attribute of the execution provider though, so using that class as a mechanism to get the options to a kernel is somewhat strange.

I think either is possible. OpKernelInfo is already passing through things from the session state, and adding a new member of ConfigOptions is the simpler update. It's also a more natural fit given it's providing state/info to the kernel.

However, if we think EP code in general would benefit from being able to use the config options (e.g. the XNNPACK EP reads info on number of threads from config options currently) it may be worth the added complexity/weirdness of updating IExecutionProvider with delayed insertion of ConfigOptions into the instance.

Motivation and Context

@yuslepukhin
Copy link
Member

Given the fact that there is not an apparent use case for EPs to make use of the options, the current approach seems right.

Update name of macro that checks message in error status to make it a little more intuitive. Simplify some existing code by using it.
@skottmckay skottmckay marked this pull request as ready for review January 10, 2024 23:28
@skottmckay skottmckay changed the title RFC: Make session configuration options available to kernels via OpKernelInfo Make session configuration options available to kernels via OpKernelInfo Jan 10, 2024
@snadampal
Copy link
Contributor

@skottmckay , thanks for extending the framework to allow opkernels receive the config options. this will make the platform specific custom opkernel implementations more cleaner.

yuslepukhin
yuslepukhin previously approved these changes Jan 11, 2024
edgchen1
edgchen1 previously approved these changes Jan 12, 2024
@jywu-msft
Copy link
Member

+@HectorSVC , @chilo-ms
it would be nice to have a way for EP's to get session config option without needing to update all the creation functions.
e.g. case is for the recent ep.context* settings in session options.

@skottmckay skottmckay dismissed stale reviews from edgchen1 and yuslepukhin via f496e80 January 12, 2024 02:36
@skottmckay
Copy link
Contributor Author

+@HectorSVC , @chilo-ms it would be nice to have a way for EP's to get session config option without needing to update all the creation functions. e.g. case is for the recent ep.context* settings in session options.

FWIW most EPs don't care. It's also a little tricky as the EP instance is created prior to the config options being available/finalized so the EP author needs to know about such restrictions. Also complicated by some EPs being behind the provider bridge.

One option is to update on a case-by-case basis to do something like what the XNNPACK EP does.

Another would be to look at adding something generic to the EP interface like 'GetSessionConfiguration' that returns a struct with relevant things (config values + things like thread info) if set. The struct allows more things to be added in the future and minimizes what the provider bridge needs to support (vs. trying to support everything in SessionOptions). Throws if not set (premature usage). In InferenceSession::Initialize we could set this struct for all EPs as the values should be final at that point.

@skottmckay skottmckay merged commit 8f2e57f into main Jan 13, 2024
90 of 92 checks passed
@skottmckay skottmckay deleted the skottmckay/AddConfigOptionsToOpKernelInfo branch January 13, 2024 00:02
mszhanyi pushed a commit that referenced this pull request Jan 15, 2024
…nfo (#18897)

### Description
<!-- Describe your changes. -->
Pass through the ConfigOptions from the session via OpKernelInfo so that
kernel behavior can be configured.

Initial usage would be to optionally enable a fast path for ARM64 bloat16 GEMM - see #17031
Other usages could be things like selected the exact implementations of the activation functions for RNN operators instead of the default approximations (e.g. use [sigmoid_exact instead of sigmoid](https://github.com/microsoft/onnxruntime/blob/2d6e2e243d1a1ab0486f4f191b61ac979c5b978e/onnxruntime/core/providers/cpu/rnn/rnn_helpers.h#L379-L382))

OpKernelInfo is already passing through things from the session state, and adding a new member of ConfigOptions
is the simpler update. It's also a more natural fit given it's providing state/info to the kernel.

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

5 participants