[ExecuTorch][#9638] Introduce Protected Method Getter in Extension.Module #10384
Conversation
…dule # Context This issue is a step of #9638. In the discussion, we want to unblock having `extension/Module` as the single source of implementation, which means that `pybindings/PyModule` should use `extension/Module` rather than its own. Although we are decouping method getter from `pybindings` implementation, method getter itself is still needed. To keep having the method getter while not exposing it, we can create a protected method getter and confine it's usage inside child classes that we are about to create. # Proposal Add a protected `get_method` to `extension.Module`, taking method name string as an input. Differential Revision: [D73473766](https://our.internmc.facebook.com/intern/diff/D73473766/) ghstack-source-id: 279669340 Pull Request resolved: #10374
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10384
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New FailuresAs of commit 52346ae with merge base 95c663e ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
This PR needs a
|
| @@ -302,5 +302,15 @@ runtime::Error Module::set_output( | |||
| output_tensor.mutable_data_ptr(), output_tensor.nbytes(), output_index); | |||
| } | |||
|
|
|||
| ET_NODISCARD inline runtime::Result<Method*> Module::get_method( | |||
There was a problem hiding this comment.
Why do we need to expose the Method? Can we make some ivars protected instead and let subclasses work with them directly, like this class itself?
Also, there's no get_-prefix naming convention for this class, it should be named just "method()", if really needed.
There was a problem hiding this comment.
Method is needed for a child class. I think I can move the function to the child class since the methods_ is already protected and the parent class is not using the function I introduced.
There was a problem hiding this comment.
@shoumikhin that should my fault i misread the scope of method list as private so I asked to put the function here.
Since it is protected, making it as a child’s private function should be a better choice.
There was a problem hiding this comment.
Now worries, you should be able to access Method via methods_.at(method_name).method in subclasses indeed.
This PR was created by the merge bot to help merge the original PR into the main branch.
ghstack PR number: #10374 by @zhenyan-zhang-meta
^ Please use this as the source of truth for the PR details, comments, and reviews
ghstack PR base: https://github.com/pytorch/executorch/tree/gh/zhenyan-zhang-meta/4/base
ghstack PR head: https://github.com/pytorch/executorch/tree/gh/zhenyan-zhang-meta/4/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head: https://github.com/pytorch/executorch/tree/gh/zhenyan-zhang-meta/4/orig
@diff-train-skip-merge