[1/N] Add BackendOptions class

[cccclai]

Stack from ghstack (oldest at bottom):

• [5/N] Add get_option/set_option APIs #11758
• [4/N] Add backend options map #11462
• [3/N] Add get_option/set_option function in backend interface #11391
• [2/N] Add option context #11390
• -> [1/N] Add BackendOptions class #11389

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

Differential Revision: D75993712

[pytorch-bot]

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

✅ No Failures

As of commit 2fda6a1 with merge base 57e0765 ():
💚 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]

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

[github-actions]

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]

Previous comment in the old PR: #11288

From @JacobSzwejbka :

use int64_t for int type.

[cccclai]

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]

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]

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.

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[kimishpatel]

this class doesnt need to be templated. No use of T

[kimishpatel]

format?

[kimishpatel]

I am a bit concerned about storing pointer to the key. Users may do things like

std::string my_key("my_key");
BackendOption().set_option<int>(my_key.c_str(), 10);

When that goes out of scope we have a problem. I suggest instead use something like const char[kMaxKeyLength] key

[kimishpatel]

The other thing I realize is why do we need BackendOption as a separate struct?

Can we not just have BackendOptions class instantiate array of keys and values? The interface there is set_option with key name and value, get_option using key name. User never really needs to get BackendOption

[cccclai]

Users don't need get backend option, but the dispatcher needs it and dispatch to the actual backend.

[cccclai]

Actually you're right. The backend options map is for dispatching. and each backend will get an array ref of backend options.

[kimishpatel]

Bassed on suggestion above

Suggested change

	void set_option(OptionKey<T> key, T value) {
	void set_option(const char [kMaxKeyLength]& key, T value) {

[kimishpatel]

set error if no capacity

[cccclai]

Yeah good point. Will update

[kimishpatel]

Dont think you need these

[cccclai]

It's just helper functions to simplify users' code.

[kimishpatel]

made some suggestions

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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