Skip to content

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

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

Open
wants to merge 3 commits into
base: gh/cccclai/27/base
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions extension/module/dynamic_backend_options_map.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@

#pragma once

#include <vector>
#include <string>
#include <initializer_list>
#include <executorch/runtime/backend/backend_options.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <vector>
#include <string>
#include <initializer_list>
#include <executorch/runtime/backend/backend_options.h>
#include <executorch/runtime/backend/backend_options.h>
#include <vector>
#include <string>
#include <initializer_list>


namespace executorch {
namespace runtime {

class DynamicBackendOptionsMap {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we somehow use only standard containers for this map in order to skip this extra class? E.g. std::vector<std::pair<const char*, ArrayRef<BackendOption>>> or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an option, not sure how simple it is compared with update API exposed in #11533 If that one is simple enough for users, we may not need to add this PR. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this PR correctly, it just makes it easier for devs to use the other update() API so that they don't need to keep the backend options preallocated and pass refs to them, but employ standard containers to keep the memory alive?

What I propose here is to skip any custom classes to keep the backend options and just use the standard containers directly. Then inside this new convenient update() override do the due conversion and call the original update().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, update the PR based on the suggestion

public:
using OptionList = std::initializer_list<BackendOption>;

DynamicBackendOptionsMap(
std::initializer_list<std::pair<const char*, OptionList>> list) {
entries_.reserve(list.size());
for (const auto& item : list) {
// Store backend name
backend_names_.push_back(item.first);
// Store options
options_storage_.push_back(std::vector<BackendOption>(item.second));
// Create Entry with stable references
entries_.push_back({
backend_names_.back().c_str(),
ArrayRef<BackendOption>(options_storage_.back().data(), options_storage_.back().size())
});
}
}

ArrayRef<Entry> entries() const {
return ArrayRef<Entry>(entries_.data(), entries_.size());
}

private:
std::vector<std::string> backend_names_;
std::vector<std::vector<BackendOption>> options_storage_;
std::vector<Entry> entries_;
};

} // namespace runtime
} // namespace executorch
7 changes: 7 additions & 0 deletions extension/module/module.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <vector>

#include <executorch/runtime/executor/program.h>
#include <executorch/extension/module/dynamic_backend_options_map.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <executorch/runtime/executor/program.h>
#include <executorch/extension/module/dynamic_backend_options_map.h>
#include <executorch/extension/module/dynamic_backend_options_map.h>
#include <executorch/runtime/executor/program.h>


namespace executorch {
namespace extension {
Expand Down Expand Up @@ -483,6 +484,12 @@ class Module {
return update("forward", backend_options);
}

ET_EXPERIMENTAL ET_NODISCARD inline runtime::Error update(
Copy link
Contributor

@shoumikhin shoumikhin Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any comments on what this API does?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comments

const std::string& method_name,
const runtime::DynamicBackendOptionsMap& backend_options) {
return update(method_name, backend_options.entries());
}

/**
* Retrieves the EventTracer instance being used by the Module.
* EventTracer is used for tracking and logging events during the execution
Expand Down
1 change: 1 addition & 0 deletions extension/module/targets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def define_common_targets():
],
exported_headers = [
"module.h",
"dynamic_backend_options_map.h",
],
visibility = [
"@EXECUTORCH_CLIENTS",
Expand Down
15 changes: 15 additions & 0 deletions extension/module/test/module_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -508,3 +508,18 @@ TEST_F(ModuleTest, TestUpdateNonExistentMethod) {
const auto update_result = module.update("nonexistent", map.entries());
EXPECT_NE(update_result, Error::Ok);
}

TEST_F(ModuleTest, TestUpdateSugarSyntax) {
Module module(stub_model_path_);
int new_num_threads = 4;

// Clean sugar syntax
const auto update_result = module.update("forward",
{
{"StubBackend", {{IntKey("NumberOfThreads"), new_num_threads}}
},
);

EXPECT_EQ(update_result, Error::Ok);
ASSERT_EQ(StubBackend::singleton().num_threads(), new_num_threads);
}
Loading