-
Notifications
You must be signed in to change notification settings - Fork 595
[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?
Conversation
The update API in method is supposed to be portable, but we can make it more user friendly for the update API in module. Add a bit sugar syntax in module to improve UX. Such that user can update backend option in module like following: ``` Module module(stub_model_path_); int new_num_threads = 4; const auto update_result = module.update("forward", { {"StubBackend", {{IntKey("NumberOfThreads"), new_num_threads}} }, ); ``` Differential Revision: [D76242292](https://our.internmc.facebook.com/intern/diff/D76242292/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11534
Note: Links to docs will display an error until the docs builds have been completed. ❌ 4 New Failures, 3 Unrelated FailuresAs of commit 8b3d944 with merge base f7cc72f ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but was present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
The update API in method is supposed to be portable, but we can make it more user friendly for the update API in module. Add a bit sugar syntax in module to improve UX. Such that user can update backend option in module like following: ``` Module module(stub_model_path_); int new_num_threads = 4; const auto update_result = module.update("forward", { {"StubBackend", {{IntKey("NumberOfThreads"), new_num_threads}} }, ); ``` Differential Revision: [D76242292](https://our.internmc.facebook.com/intern/diff/D76242292/) ghstack-source-id: 289276526 Pull Request resolved: #11534
This PR needs a
|
This pull request was exported from Phabricator. Differential Revision: D76242292 |
#include <vector> | ||
#include <string> | ||
#include <initializer_list> | ||
#include <executorch/runtime/backend/backend_options.h> |
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.
#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> |
extension/module/module.h
Outdated
#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 comment
The reason will be displayed to describe this comment to others. Learn more.
#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> |
extension/module/module.h
Outdated
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Update the comments
namespace executorch { | ||
namespace runtime { | ||
|
||
class DynamicBackendOptionsMap { |
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.
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?
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.
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 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().
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.
Ah I see, update the PR based on the suggestion
The update API in method is supposed to be portable, but we can make it more user friendly for the update API in module. Add a bit sugar syntax in module to improve UX. Such that user can update backend option in module like following: ``` Module module(stub_model_path_); int new_num_threads = 4; const auto update_result = module.update("forward", { {"StubBackend", {{IntKey("NumberOfThreads"), new_num_threads}} }, ); ``` Differential Revision: [D76242292](https://our.internmc.facebook.com/intern/diff/D76242292/) [ghstack-poisoned]
Pull Request resolved: #11534 The update API in method is supposed to be portable, but we can make it more user friendly for the update API in module. Add a bit sugar syntax in module to improve UX. Such that user can update backend option in module like following: ``` Module module(stub_model_path_); int new_num_threads = 4; const auto update_result = module.update("forward", { {"StubBackend", {{IntKey("NumberOfThreads"), new_num_threads}} }, ); ``` ghstack-source-id: 290073340 @exported-using-ghexport Differential Revision: [D76242292](https://our.internmc.facebook.com/intern/diff/D76242292/)
This pull request was exported from Phabricator. Differential Revision: D76242292 |
The update API in method is supposed to be portable, but we can make it more user friendly for the update API in module. Add a bit sugar syntax in module to improve UX. Such that user can update backend option in module like following: ``` Module module(stub_model_path_); int new_num_threads = 4; const auto update_result = module.update("forward", { {"StubBackend", {{IntKey("NumberOfThreads"), new_num_threads}} }, ); ``` Differential Revision: [D76242292](https://our.internmc.facebook.com/intern/diff/D76242292/) [ghstack-poisoned]
Pull Request resolved: #11534 The update API in method is supposed to be portable, but we can make it more user friendly for the update API in module. Add a bit sugar syntax in module to improve UX. Such that user can update backend option in module like following: ``` Module module(stub_model_path_); int new_num_threads = 4; const auto update_result = module.update("forward", { {"StubBackend", {{IntKey("NumberOfThreads"), new_num_threads}} }, ); ``` ghstack-source-id: 290372285 @exported-using-ghexport Differential Revision: [D76242292](https://our.internmc.facebook.com/intern/diff/D76242292/)
This pull request was exported from Phabricator. Differential Revision: D76242292 |
Stack from ghstack (oldest at bottom):
The update API in method is supposed to be portable, but we can make it more user friendly for the update API in module. Add a bit sugar syntax in module to improve UX. Such that user can update backend option in module like following:
Differential Revision: D76242292