Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[torchlib] Allow calling shared functions from other onnx functions #834

Open
Tracked by #357
justinchuby opened this issue Jul 6, 2023 · 4 comments
Open
Tracked by #357

[torchlib] Allow calling shared functions from other onnx functions #834

justinchuby opened this issue Jul 6, 2023 · 4 comments
Labels
enhancement New feature or request topic: torch_lib Related to the torch/aten function lib in development
Milestone
0.2

Comments

@justinchuby
Copy link
Collaborator

Currently the torchscript evaluator does not support calling OnnxFunctions from within an OnnxFunction. We should enable this to allow shared implementations. To do this, we will need to keep references of the called functions inside an OnnxFunction.
@justinchuby justinchuby added the topic: torch_lib Related to the torch/aten function lib in development label Jul 6, 2023
@BowenBao
Copy link
Contributor

BowenBao commented Jul 7, 2023

Done by #725 ?

@justinchuby
Copy link
Collaborator Author

Ahh that’s right! I remember this pr but got a test error when I did this. I must have confused myself. Thanks!

@justinchuby justinchuby mentioned this issue Jul 7, 2023
Open
@justinchuby
Copy link
Collaborator Author

justinchuby commented Jul 8, 2023 •
edited
Loading

So we need to use add_module_call it seems? Looks like the graph builder is not handling cases where a function calls another (when using OnnxFunction to_function_proto).

I can think of two options:

  1. Create a new API in OnnxFunction to return a list of function protos used by the function (recursively) (deduplication would be an issue)
  2. Record the functions in the Graph on the fly Record OnnxFunctions in called functions #629
  3. Do what to_model_proto does (I haven't looked)

@justinchuby justinchuby reopened this Jul 8, 2023
@justinchuby justinchuby self-assigned this Jul 8, 2023
@justinchuby justinchuby mentioned this issue Jul 8, 2023
Draft
@BowenBao
Copy link
Contributor

@justinchuby We don't need add_module_call for this, the rest makes sense. There is existing api IRFunction.to_graph_and_functions / IRFunction.called_functions used by OnnxFunction.to_model_proto to grab inner functions. I believe all we need is exposing the api at OnnxFunction level and use it in TorchScriptGraph.add_function_call.

@justinchuby justinchuby mentioned this issue Jul 11, 2023
Open
@justinchuby justinchuby added the enhancement New feature or request label Aug 4, 2023
@justinchuby justinchuby mentioned this issue Aug 8, 2023
Merged
@justinchuby justinchuby added this to the 0.2 milestone Aug 31, 2023
@justinchuby justinchuby mentioned this issue Oct 23, 2023
Merged
justinchuby added a commit that referenced this issue Oct 24, 2023
@justinchuby
Create the Rank and IsScalar shared functions | feat(torchlib) (#1105)
9fb0a7d 
This change introduces two shared operators `Rank` and `IsScalar`. They
are used to replace the `Size(Shape())` pattern for code reuse and
readability.

I used a hack to always include these shared functions in the model
proto because without #834 we cannot dynamically add these functions to
the model as they are used. I added a TODO for this.

The first usage is in `aten_all`. I will update the rest of the
functions in a separate PR.

#1095
@BowenBao BowenBao mentioned this issue Feb 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request topic: torch_lib Related to the torch/aten function lib in development
Projects
None yet
Milestone
0.2
Development

No branches or pull requests
2 participants
@BowenBao @justinchuby