Support calling custom method names on PyTorch module #99
Conversation
iceychris
commented
Apr 8, 2023
•
iceychris
commented
- basic support
- test with dict inputs
- test with list inputs
- test default method name ("forward")
- test custom method name
- format
iceychris
force-pushed
the
feature/support-custom-method-names
branch
from
75f87cd
to
019793a
Compare
iceychris
force-pushed
the
feature/support-custom-method-names
branch
3 times, most recently
from
dbecc04
to
40b305b
Compare
iceychris
force-pushed
the
feature/support-custom-method-names
branch
from
40b305b
to
c6a9e1c
Compare
|
@dyastremsky @tanmayv25 Mind looking into this? This PR should be ready. I tested locally by building & running |
iceychris
force-pushed
the
feature/support-custom-method-names
branch
from
c6a9e1c
to
6c0b463
Compare
|
Thanks for submitting this PR and tagging us, Christian! We'll need a Contributor License Agreement (CLA) here to merge these changes. You can see how to submit it here. That's also got directions on formatting this code to be aligned with the rest of the repo. In addition to reviewing your ticket, the other thing we'll need to look into once we get your CLA is adding tests. We appreciate you manually testing, we do need automated tests to ensure this passes and doesn't break in the future. You can see how this is done in the server qa folder here. The model generation is done in the qa's common folder. If you're unable to add the tests, we may be able to do so, though then we'll need time to get to this ticket. |
|
@iceychris Thanks for your contribution! We are still waiting from the signed CLA from you to proceed with it further. Please let me know once it is completed. I can work on adding test cases for this on our CI if you'd like. |
src/libtorch.cc
Outdated
| @@ -764,7 +799,8 @@ ModelInstanceState::ValidateInputs(const size_t expected_input_cnt) | |||
| // configuration specifies only those. | |||
| std::vector<std::string> allowed_inputs; | |||
|
|
|||
| const torch::jit::Method& method = torch_model_->get_method("forward"); | |||
| const torch::jit::Method& method = | |||
There was a problem hiding this comment.
Add error checking here for case the model_state_->ModuleMethodName() is not available in the model.
Otherwise the server will see a core dump..
There was a problem hiding this comment.
@iceychris Did you get a chance to add the exception handling? I can take over if you don't mind?
There was a problem hiding this comment.
Not yet, I'm currently busy, so feel free to go ahead 👍🏾
|
@iceychris Can you let this PR finish merging? This is a very important feature. |
iceychris
force-pushed
the
feature/support-custom-method-names
branch
from
6c0b463
to
e4cc7a6
Compare
…ton-inference-server/server#5209) Signed-off-by: Christian Bruckdorfer <[email protected]>
iceychris
force-pushed
the
feature/support-custom-method-names
branch
from
e4cc7a6
to
11fb866
Compare
|
@tanmayv25 I signed the CLA along with the commit and rebased onto |
