[1/N] Add backend option #11288

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.

Sign up for GitHub

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

Jump to bottom

Closed

cccclai wants to merge 1 commit into pytorch:main from cccclai:export-D75770142

Contributor

cccclai commented Jun 2, 2025 •

edited

Loading

Summary: Introduce backend option as discussed in #10216

Differential Revision: D75770142

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

pytorch-bot bot commented Jun 2, 2025 •

edited

Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11288

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit e91888c with merge base 8e2737c ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot added the CLA Signed label

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

facebook-github-bot added the fb-exported label

cccclai changed the title ~~Add backend option~~ [1/N] Add backend option

cccclai force-pushed the export-D75770142 branch from ca30f97 to 359364a Compare

June 2, 2025 18:31

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

mergennachin requested review from digantdesai, mergennachin, larryliu0820, kimishpatel, JacobSzwejbka and mcr229

June 2, 2025 18:33

cccclai added the release notes: api label

cccclai added a commit to cccclai/executorch-1 that referenced this pull request


          Add backend option (pytorch#11288)

01bae44

Summary:

Introduce backend option as discussed in pytorch#10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

Differential Revision: D75770142

cccclai force-pushed the export-D75770142 branch from 359364a to 01bae44 Compare

June 2, 2025 18:44

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

cccclai added a commit to cccclai/executorch-1 that referenced this pull request


          Add backend option (pytorch#11288)

1a68abc

Summary:

Introduce backend option as discussed in pytorch#10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

Differential Revision: D75770142

cccclai force-pushed the export-D75770142 branch from 01bae44 to 1a68abc Compare

June 2, 2025 18:51

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

cccclai added a commit to cccclai/executorch-1 that referenced this pull request


          Add backend option (pytorch#11288)

fbdb2b5

Summary:

Introduce backend option as discussed in pytorch#10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

Differential Revision: D75770142

cccclai force-pushed the export-D75770142 branch from 1a68abc to fbdb2b5 Compare

June 2, 2025 18:53

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

cccclai added a commit to cccclai/executorch-1 that referenced this pull request


          Add backend option (pytorch#11288)

7be9c3a

Summary:

Introduce backend option as discussed in pytorch#10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

Differential Revision: D75770142

cccclai force-pushed the export-D75770142 branch from fbdb2b5 to 7be9c3a Compare

June 2, 2025 19:07

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

cccclai added a commit to cccclai/executorch-1 that referenced this pull request


          Add backend option (pytorch#11288)

2d0c08c

Summary:

Introduce backend option as discussed in pytorch#10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

Differential Revision: D75770142

cccclai force-pushed the export-D75770142 branch from 7be9c3a to 2d0c08c Compare

June 2, 2025 19:36

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

cccclai added a commit to cccclai/executorch-1 that referenced this pull request


          Add backend option (pytorch#11288)

d3a4574

Summary:

Introduce backend option as discussed in pytorch#10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

Differential Revision: D75770142

cccclai force-pushed the export-D75770142 branch from 2d0c08c to d3a4574 Compare

June 2, 2025 19:57

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

cccclai added a commit to cccclai/executorch-1 that referenced this pull request


          Add backend option (pytorch#11288)

7d42f0e

Summary:

Introduce backend option as discussed in pytorch#10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

Differential Revision: D75770142

cccclai force-pushed the export-D75770142 branch from d3a4574 to 7d42f0e Compare

June 2, 2025 21:33

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142


          Add backend option (pytorch#11288)

e91888c

Summary:

Introduce backend option as discussed in pytorch#10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack.

Differential Revision: D75770142

cccclai force-pushed the export-D75770142 branch from 7d42f0e to e91888c Compare

June 2, 2025 22:09

Contributor

facebook-github-bot commented Jun 2, 2025

This pull request was exported from Phabricator. Differential Revision: D75770142

Contributor

JacobSzwejbka commented Jun 2, 2025

Will review tomorrow

JacobSzwejbka reviewed

View reviewed changes

runtime/backend/backend_option.h

+              // Union-like container for option values (only one member is valid per option)
+              struct OptionValue {
+                bool bool_value; // Storage for boolean values
+                int int_value; // Storage for integer values

Contributor

JacobSzwejbka Jun 2, 2025

int64_t

Contributor

mergennachin commented Jun 4, 2025 •

edited

Loading

I don't think we need to define a generic BackendOption that takes arbitrary key value. Each backend owners could define their options instead. Vulkan will define its own config structure, Core ML will define its own config structure. Wdyt?

@JacobSzwejbka, @mcr229, @kimishpatel

Something like this:

BackendInterface { // parent interface
  virtual Error setOption(const BackendOptions& options) = 0;
}

struct BackendOptions {  // Empty parent struct
  virtual ~BackendOptions() = default;
};

Each individual backends define their own configs

struct VulkanBackendOptions : public BackendOptions {
  const char* metadata[20] = "";
  int thread_count = 1;
  // ... other Vulkan-specific options
};

VulkanBackend extends BackendInterface {
  Error setOption(const BackendOptions& options) override {
    auto* vulkan_opts = dynamic_cast<const VulkanBackendOptions*>(&options);
  }
}

// User code

VulkanBackendOptions opts;
opts.thread_count = 5;
auto backend = std::make_unique<VulkanBackend>();
backend->setOption(opts);  //

Contributor Author

cccclai commented Jun 4, 2025

I don't think we need to define a generic BackendOption that takes arbitrary key value. Each backend owners could define their options instead. Vulkan will define its own config structure, Core ML will define its own config structure. Wdyt?

@JacobSzwejbka, @mcr229, @kimishpatel

Something like this:
BackendInterface { // parent interface
  virtual Error setOption(const BackendOptions& options) = 0;
}

struct BackendOptions {  // Empty parent struct
  virtual ~BackendOptions() = default;
};
Each individual backends define their own configs
struct VulkanBackendOptions : public BackendOptions {
  const char* metadata[20] = "";
  int thread_count = 1;
  // ... other Vulkan-specific options
};
VulkanBackend extends BackendInterface {
  Error setOption(const BackendOptions& options) override {
    auto* vulkan_opts = dynamic_cast<const VulkanBackendOptions*>(&options);
  }
}
// User code
VulkanBackendOptions opts;
opts.thread_count = 5;
auto backend = std::make_unique<VulkanBackend>();
backend->setOption(opts);  // 

I thought about this idea at the beginning, but it means that users code needs to link to a specific backend, otherwise the runner code will fail to build. Currently the same user code can be backend agnostic, meaning the same code can still run with different backends with/without linking the backend. As the example, the portable runner can work with/without xnnpack https://github.com/pytorch/executorch/blob/main/examples/xnnpack/executor_runner/targets.bzl.

It also exposes the backend specific symbol to users, and the contract interface is a bit looser. Previously, The contract is like backend <-> ET <-> backend, and if we change it to this, it can be user <-> backend. And users can pass arbitrary things to backend. Users can run backend->init, backend->execute in their own code too, and it will be out of our control.

With the proposed solution (#10216), the user code can still be backend agnostic, and the backend option is still optional. We are still on the hook of the contract.

Contributor Author

cccclai commented Jun 4, 2025 •

edited

Loading

Btw I was thinking using ghstack such that I can export a stack of PRs...but it means this PR will be closed and a new PR will be created. I will copy the comments over just so we keep the history.

Edit: the new pr is here #11389, I'm closing this PR and please leave comments in the new PR

cccclai closed this

cccclai mentioned this pull request

[1/N] Add BackendOptions class #11389

Open

digantdesai reviewed

View reviewed changes

runtime/backend/backend_option.h

+              enum class OptionType { BOOL, INT, STRING };
+              // Union-like container for option values (only one member is valid per option)
+              struct OptionValue {

Contributor

digantdesai Jun 5, 2025 •

edited

Loading

Why not use a union? Or structure with a template?

Contributor

kimishpatel Jun 6, 2025

or std::variant that can hold only bool, int and char *?

Contributor Author

cccclai Jun 6, 2025

union can be a good option, let me try

kimishpatel reviewed

View reviewed changes

Contributor

kimishpatel left a comment

Left a few comments

runtime/backend/backend_option.h

+              #include <cstring>
+              namespace executorch {
+              namespace ET_RUNTIME_NAMESPACE {

Contributor

kimishpatel Jun 6, 2025

why this capitalized?

Contributor Author

cccclai Jun 6, 2025

It's used across the whole ExecuTorch stack, but I find now it's not needed so switch to executorch::runtime namespace

runtime/backend/backend_option.h

+              };
+              // Supported option data types
+              enum class OptionType { BOOL, INT, STRING };

Contributor

kimishpatel Jun 6, 2025

Just use std::varaint

runtime/backend/backend_option.h

+              // Represents a single backend configuration option
+              struct BackendOption {
+                const char* key; // Name of the option

Contributor

kimishpatel Jun 6, 2025

you can also do

struct BackendOptions {
  const char* key;
  std::variant<int, bool, char *> value;
}

Contributor Author

cccclai Jun 6, 2025

It will introduce std library I assume?

runtime/backend/backend_option.h

+                /// Sets or updates a boolean option
+                /// @param key: Typed option key
+                /// @param value: Boolean value to set
+                void set_option(OptionKey<bool> key, bool value) {

Contributor

kimishpatel Jun 6, 2025

Just make this set_option<T>(BackendOption<T> key_value)...

If tomorrow you expand BackendOption to have other types like float or int8 you just have to update allowed types in BackendOpton.

Contributor

kimishpatel Jun 6, 2025

and assert to on allowable types

Contributor Author

cccclai Jun 6, 2025

BackendOptions is a list of BackendOption, and each BackendOption can be different type (Like cpu: {thread: 2, profile: true}). If we make BackendOption a template, then BackendOptions can only hold the same type of BackendOption, which is not we would like.

runtime/backend/backend_option.h

+              enum class OptionType { BOOL, INT, STRING };
+              // Union-like container for option values (only one member is valid per option)
+              struct OptionValue {

Contributor

kimishpatel Jun 6, 2025

or std::variant that can hold only bool, int and char *?

runtime/backend/backend_option.h

+               private:
+                BackendOption options[MaxCapacity]{}; // Storage for options
+                size_t size; // Current number of options

Contributor

kimishpatel Jun 6, 2025

size_ for private variables.

runtime/backend/backend_option.h

Comment on lines +134 to +137

+                executorch::runtime::Error get_option_internal(
+                    const char* key,
+                    OptionType expected_type,
+                    OptionValue& out) const {

Contributor

kimishpatel Jun 6, 2025

Is it necessary that type is specified? If we use std::variant we can just make this get_optional_internal(const char* key, BackendOption& option); User can be responsible for type check. Not sure why we need to provide it

Contributor Author

cccclai Jun 6, 2025

std::variant is part of the std library, and this code needs to stay in core

Contributor Author

cccclai commented Jun 6, 2025

Address comments in #11389, please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

digantdesai digantdesai left review comments

kimishpatel kimishpatel left review comments

JacobSzwejbka JacobSzwejbka left review comments

mergennachin Awaiting requested review from mergennachin

larryliu0820 Awaiting requested review from larryliu0820

mcr229 Awaiting requested review from mcr229

Labels

CLA Signed fb-exported release notes: api