-
Notifications
You must be signed in to change notification settings - Fork 600
[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
base: gh/cccclai/27/base
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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> | ||
|
||
namespace executorch { | ||
namespace runtime { | ||
|
||
class DynamicBackendOptionsMap { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,6 +15,7 @@ | |||||||||
#include <vector> | ||||||||||
|
||||||||||
#include <executorch/runtime/executor/program.h> | ||||||||||
#include <executorch/extension/module/dynamic_backend_options_map.h> | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
namespace executorch { | ||||||||||
namespace extension { | ||||||||||
|
@@ -483,6 +484,12 @@ class Module { | |||||||||
return update("forward", backend_options); | ||||||||||
} | ||||||||||
|
||||||||||
ET_EXPERIMENTAL ET_NODISCARD inline runtime::Error update( | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any comments on what this API does? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.