[1/N] Add BackendOptions class #11389

cccclai · 2025-06-05T04:09:24Z

Stack from ghstack (oldest at bottom):

Introduce backend option as discussed in #10216

Step 1: Introducd Backend Option class

In later stage, it will be plugged in with the rest of the stack. BackendOptions is pretty much a list of BackendOption, and backend option is a key value pair. The key is a string, and the value can be 3 different types, including bool, string and int.

Differential Revision: D75993712

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) [ghstack-poisoned]

pytorch-bot · 2025-06-05T04:09:28Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ You can merge normally! (3 Unrelated Failures)

As of commit 943fe96 with merge base 608a745 ():

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

pull / test-models-linux (ic3, xnnpack-quantization-delegation, linux.2xlarge) / linux-job (gh) (detected as infra flaky with no log or failing log classifier)
pull / test-models-linux (mobilebert, portable, linux.2xlarge) / linux-job (gh) (detected as infra flaky with no log or failing log classifier)

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

Apple / build-benchmark-app / macos-job (gh) (trunk failure)
RuntimeError: Command bash /Users/runner/work/_temp/exec_script failed with exit code 65

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

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) ghstack-source-id: 288273758 Pull Request resolved: #11389

facebook-github-bot · 2025-06-05T04:09:34Z

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

github-actions · 2025-06-05T04:10:06Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

cccclai · 2025-06-05T04:12:12Z

Previous comment in the old PR: #11288

From @JacobSzwejbka :

use int64_t for int type.

cccclai · 2025-06-05T04:12:46Z

From @mergennachin:

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);  // 

cccclai · 2025-06-05T04:13:36Z

My response:

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.

kimishpatel · 2025-06-06T02:52:21Z

I think what @mergennachin suggested makes sense. If user code is setting backend specific option they already have to know what backend they are talking about, unless some options are backend agnostic.

Other piece of the puzzle is how the user will update this. And from #10216 it seems that we will pipe update (I would call set_backend_options) method that will set all the options. But part that is not clear to me is how would user indicate which backend they setting this option for. So somehow we need a string identifer for the backends that can be used to figure out which backend's update method be called. @mergennachin in this case we cannot quite have what you proposed. Imagine you have a pointer to an instance of VulkanBackendOptions. You pass this to runtime's update method which accepts BackendOptions*. Eventually you want to call VulkanBackend's update options method so you need to cast BackendOptions* to VulkanBackendOptions*. You have two choices either do static_cast or dynamic_cast. The later requires rtti which we dont allow. For static_cast, it has to be safe and you need to know that BackendOption* is actually pointing to VulkanBackendOption. To do that you need to have BackendOptions store tag containing what is the type of the derived class. If we do that, then what you suggest can work.

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) [ghstack-poisoned]

facebook-github-bot · 2025-06-06T23:50:17Z

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

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) [ghstack-poisoned]

facebook-github-bot · 2025-06-10T18:39:56Z

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

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) [ghstack-poisoned]

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. BackendOptions is pretty much a list of BackendOption, and backend option is a key value pair. The key is a string, and the value can be 3 different types, including bool, string and int. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712) [ghstack-poisoned]

facebook-github-bot · 2025-06-17T18:23:16Z

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

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. BackendOptions is pretty much a list of BackendOption, and backend option is a key value pair. The key is a string, and the value can be 3 different types, including bool, string and int. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712) [ghstack-poisoned]

facebook-github-bot · 2025-06-18T05:54:21Z

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

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. BackendOptions is pretty much a list of BackendOption, and backend option is a key value pair. The key is a string, and the value can be 3 different types, including bool, string and int. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712) [ghstack-poisoned]

facebook-github-bot · 2025-06-18T06:05:48Z

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

mergennachin

See inline

runtime/backend/options.h

mergennachin · 2025-06-20T15:03:40Z

runtime/backend/options.h

+
+// Union replaced with std::variant
+using OptionValue = std::variant<bool, int, const char*>;
+


Document the safety and lifecycle of string types

All string keys and values must have static storage duration (string literals, static const char arrays, or global constants). The BackendOptions class does NOT take ownership of strings.

For the actual value itself, I think we should do this where std::variant itself holds array of chars rather than pointer to char. Like this https://godbolt.org/z/odTc9ex96

mergennachin · 2025-06-20T15:09:44Z

runtime/backend/options.h

+constexpr OptionKey<const char*> StrKey(const char* k) {
+  return OptionKey<const char*>(k);
+}


Can you force to accept only string literals with this?

template <size_t N> constexpr OptionKey<const char*> StrKey(const char (&literal)[N]) { return OptionKey<const char*>(literal); }

mergennachin · 2025-06-20T15:18:05Z

runtime/backend/options.h

+
+  // Type-safe setters
+  template <typename T>
+  void set_option(OptionKey<T> key, T value) {


rename this to set_option_impl and make it a private function. And add type safe public APIs to enforce key is always safe string... And value is also safe in the case of string.

// String options - only accepts string literals/arrays template <size_t KeyN, size_t ValueN> Error set_option( const char (&key)[KeyN], const char (&value)[ValueN] ) noexcept { return set_option_impl(key, static_cast<const char*>(value)); } // Non-string options template <size_t N> Error set_option(const char (&key)[N], bool value) noexcept { return set_option_impl(key, value); } template <size_t N> Error set_option(const char (&key)[N], int value) noexcept { return set_option_impl(key, value); }

kimishpatel · 2025-06-20T15:41:27Z

runtime/backend/options.h

+
+// Strongly-typed option key template
+template <typename T>
+struct OptionKey {


I dont think we need OptionKey as a separate class. Just use const char * in the BackendOption class.

However, the thing that worries me the most is by owning const char* we are giving users a footgun. They can allocate pointer to the backend option name on stack, pass the pointer around and it will go out-of-scope. If this results in segfault that might be your best option. Worst can lead to undefined behaviors.

I would suggest that we just store const char[kMaxLegthOfBackendOptionName].

You resolved this comment but I dont see it being addressed

Looks like you addressed but kept OptionKey. please remove

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. BackendOptions is pretty much a list of BackendOption, and backend option is a key value pair. The key is a string, and the value can be 3 different types, including bool, string and int. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712) [ghstack-poisoned]

facebook-github-bot · 2025-06-23T05:07:54Z

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

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. BackendOptions is pretty much a list of BackendOption, and backend option is a key value pair. The key is a string, and the value can be 3 different types, including bool, string and int. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712) [ghstack-poisoned]

cccclai · 2025-06-23T05:11:24Z

@kimishpatel @mergennachin thanks for the feedback! I've addressed most of them. If there is anything I miss, let me know

facebook-github-bot · 2025-06-23T05:11:48Z

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

mergennachin

See inline, approving but inline comments should be addressed

mergennachin · 2025-06-23T14:38:01Z

runtime/backend/options.h

+   */
+  template <size_t N>
+  Error set_option(const char (&key)[N], bool value) noexcept {
+    ET_CHECK_MSG(N <= kMaxOptionKeyLength, "Option key is too long");


static_assert(N <= kMaxOptionKeyLength, "Option key is too long");

for compile time check, as opposed to runtime check

why is there KeyLen here? Can it not be just max length? Like why are we templating this on keylength? If it is to allow for users to construct const char[10] vs. const char [5], I would say just make recommended use via const char[kMaxOptionKeyLength]? Feels like that is simler

mergennachin · 2025-06-23T14:39:41Z

runtime/backend/options.h

+   */
+  template <size_t N>
+  Error set_option(const char (&key)[N], int value) noexcept {
+    ET_CHECK_MSG(N <= kMaxOptionKeyLength, "Option key is too long");


mergennachin · 2025-06-23T14:39:46Z

runtime/backend/options.h

+   */
+  template <size_t N>
+  Error set_option(const char (&key)[N], const char* value) noexcept {
+    ET_CHECK_MSG(N <= kMaxOptionKeyLength, "Option key is too long");


mergennachin · 2025-06-23T14:41:59Z

runtime/backend/options.h

+      strncpy(new_option.key, key, kMaxOptionKeyLength - 1);
+      new_option.key[kMaxOptionKeyLength - 1] = '\0';


const size_t key_len = std::strlen(key); const size_t copy_len = std::min(key_len, kMaxOptionKeyLength - 1); std::memcpy(new_option.key, key, copy_len); new_option.key[copy_len] = '\0';

mergennachin · 2025-06-23T14:42:51Z

runtime/backend/options.h

+   *
+   * @return A const Span containing all BackendOption entries
+   */
+  executorch::runtime::Span<BackendOption> view() const {


executorch::runtime::Span<const BackendOption> view() const

mergennachin · 2025-06-23T14:43:59Z

runtime/backend/options.h

+   * Default constructor - initializes with zero options.
+   */
+  BackendOptions() : size_(0) {}
+


Potentially have this

/** * Copy constructor */ BackendOptions(const BackendOptions& other) : size_(other.size_) { for (size_t i = 0; i < size_; ++i) { options_[i] = other.options_[i]; } } /** * Copy assignment operator */ BackendOptions& operator=(const BackendOptions& other) { if (this != &other) { size_ = other.size_; for (size_t i = 0; i < size_; ++i) { options_[i] = other.options_[i]; } } return *this; }

kimishpatel · 2025-06-23T14:50:20Z

@mergennachin I am concerned about lifecycle and potential footgun it introduces when setting string values. So I would like to resolve that before we land

mergennachin · 2025-06-23T15:05:40Z

@kimishpatel

Can we just always require string value to be a string literals, static const char arrays, or global constants in the API like this?

#11389 (comment)

kimishpatel · 2025-06-23T15:20:54Z

runtime/backend/options.h

+   *
+   * @return A mutable Span containing all BackendOption entries
+   */
+  executorch::runtime::Span<BackendOption> view() {


maybe make it explicit that this is a mutable view.

kimishpatel · 2025-06-23T15:26:22Z

runtime/backend/options.h

+   * exist, Error::InvalidArgument if type doesn't match
+   */
+  template <typename T, size_t KeyLen>
+  Error get_option(const char (&key)[KeyLen], T& out) const {


same as before on keylen

kimishpatel · 2025-06-23T15:27:13Z

runtime/backend/test/backend_options_test.cpp

+  // Integer get should succeed with correct value
+  EXPECT_EQ(options.get_option("flag", ival), Error::Ok);
+  EXPECT_EQ(ival, 123);
+}


Add tests for mutability of the options now that you have mutable view

kimishpatel

I think we should make option itself have char array instead of char*

kimishpatel · 2025-06-23T16:03:34Z

@kimishpatel

Can we just always require string value to be a string literals, static const char arrays, or global constants in the API like this?

#11389 (comment)

Yeah those are fine options. I just want API to make it clear, like what you suggested in the comment (although I think length specifier, for option and value, being template param seems a bit weird. Just have a max longest length as some const)

Introduce backend option as discussed in #10216 Step 1: Introducd Backend Option class In later stage, it will be plugged in with the rest of the stack. BackendOptions is pretty much a list of BackendOption, and backend option is a key value pair. The key is a string, and the value can be 3 different types, including bool, string and int. Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712/) Differential Revision: [D75993712](https://our.internmc.facebook.com/intern/diff/D75993712) [ghstack-poisoned]

facebook-github-bot · 2025-06-23T18:38:48Z

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

cccclai · 2025-06-23T18:40:39Z

New comments are addressed, let me know your thoughts

kimishpatel · 2025-06-23T19:14:58Z

runtime/backend/options.h

+   * @return Error::Ok on success, Error::InvalidArgument if storage is full
+   */
+  template <size_t N>
+  Error set_option(const char (&key)[N], bool value) noexcept {


Do we need N? It will just create N different methods for many possible lengths. My suggestion is to remove N as template param and expect callsites to be something like

char my_option_name[kMaxOptionKeyLength] = "use_gpu"; ...set_option(my_option_name, True)

kimishpatel · 2025-06-23T19:15:43Z

runtime/backend/options.h

+    // Create a fixed-size array and copy the string
+    std::array<char, kMaxOptionValueLength> arr;
+    strncpy(arr.data(), value, kMaxOptionValueLength - 1);
+    arr[kMaxOptionValueLength - 1] = '\0'; // Ensure null termination


Using the suggestion I mentioned above will also not require you to do this. ALthough its not a bit deal

kimishpatel

Looks good. I still think we should not need N or KeyLen template params. So I would suggest to fix that.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 5, 2025

facebook-github-bot added the fb-exported label Jun 5, 2025

cccclai mentioned this pull request Jun 5, 2025

[1/N] Add backend option #11288

Closed

This was referenced Jun 5, 2025

[2/N] Add option context #11390

Open

[3/N] Add get_option/set_option function in backend interface #11391

Open

cccclai requested review from mergennachin, kimishpatel, JacobSzwejbka, digantdesai, mcr229, metascroy and SS-JIA June 5, 2025 04:16

This was referenced Jun 6, 2025

[4/N] Add backend options map #11462

Open

[5/N] Add update in method #11463

Open

This was referenced Jun 10, 2025

[6/N] Add update in module #11533

Open

[7/N] Add sugar syntax for module.update #11534

Open

This was referenced Jun 17, 2025

Support Span to construct from a single element similar to ArrayRef #11757

Merged

[5/N] Add get_option/set_option APIs #11758

Open

mergennachin reviewed Jun 20, 2025

View reviewed changes

kimishpatel reviewed Jun 20, 2025

View reviewed changes

mergennachin approved these changes Jun 23, 2025

View reviewed changes

kimishpatel reviewed Jun 23, 2025

View reviewed changes

kimishpatel requested changes Jun 23, 2025

View reviewed changes

kimishpatel reviewed Jun 23, 2025

View reviewed changes

kimishpatel approved these changes Jun 23, 2025

View reviewed changes

zingo added the ciflow/trunk label Jun 23, 2025


		// Union replaced with std::variant
		using OptionValue = std::variant<bool, int, const char*>;

		strncpy(new_option.key, key, kMaxOptionKeyLength - 1);
		new_option.key[kMaxOptionKeyLength - 1] = '\0';

[1/N] Add BackendOptions class #11389

Are you sure you want to change the base?

[1/N] Add BackendOptions class #11389

Conversation

cccclai commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ You can merge normally! (3 Unrelated Failures)

Uh oh!

facebook-github-bot commented Jun 5, 2025

Uh oh!

github-actions bot commented Jun 5, 2025

This PR needs a release notes: label

Uh oh!

cccclai commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cccclai commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cccclai commented Jun 5, 2025

Uh oh!

kimishpatel commented Jun 6, 2025

Uh oh!

facebook-github-bot commented Jun 6, 2025

Uh oh!

facebook-github-bot commented Jun 10, 2025

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

facebook-github-bot commented Jun 18, 2025

Uh oh!

facebook-github-bot commented Jun 18, 2025

Uh oh!

mergennachin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 23, 2025

Uh oh!

cccclai commented Jun 23, 2025

Uh oh!

facebook-github-bot commented Jun 23, 2025

Uh oh!

mergennachin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cccclai commented Jun 5, 2025 •

edited

Loading

pytorch-bot bot commented Jun 5, 2025 •

edited

Loading

This PR needs a `release notes:` label

cccclai commented Jun 5, 2025 •

edited

Loading

cccclai commented Jun 5, 2025 •

edited

Loading

mergennachin commented Jun 23, 2025 •

edited

Loading