[1/N] Add BackendOptions class

[cccclai]

Stack from ghstack (oldest at bottom):

• [5.1/ N] set_option/get_option API with {backend_name, backend options} only #11865
• [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 8c35e52 with merge base 222d9e3 ():
💚 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

[cccclai]

New comments are addressed, let me know your thoughts

[kimishpatel]

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]

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.

[cccclai]

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

I remove the template N, given that it might generate too many code and causing too much size regression. However the cost is that the key length check is runtime instead of compile time, which I think maybe fine

[facebook-github-bot]

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

[kimishpatel]

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

I remove the template N, given that it might generate too many code and causing too much size regression. However the cost is that the key length check is runtime instead of compile time, which I think maybe fine

There shouldnt be any key lengths check really though? Your fn would be get_option(const char optin_name[kMaxLength]...)

[kimishpatel]

See my comment. I didt mean this

[facebook-github-bot]

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

[cccclai]

Discuss offline with @kimishpatel and experiement with set_option(const char optin_name[kMaxLength]...), looks like array length is still not known at compile time, hence switching back to template base

[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