[NNC] Add python bindings for Compute2 #59350
Conversation
[ghstack-poisoned]
facebook-github-bot
added
oncall: jit
💊 CI failures summary and remediationsAs of commit 6d44c06 (more details on the Dr. CI page):
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
@huiguoo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| py::return_value_policy::reference); | ||
|
|
||
| te.def( | ||
| "Compute2", |
There was a problem hiding this comment.
Why not add this as another case under the def above for Compute?
There was a problem hiding this comment.
As the case of dim_args.size()>4? It seems unnecessary (dim_args.size()>4) to use Compute2-like API (function argument being a vector). Also, we may not want the user to check the number of dim_args before they use Compute.
There was a problem hiding this comment.
I didn't mean for the user to check the number of dim_args. The user will just pass a func to Compute. That func either takes in 1, 2, 3, or 4 parameters (as is currently the case) or takes in a vector (which is the case you are trying to add) if the number of args is > 4.
Are you suggesting that the user might want to use this API even when the number of args is <= 4?
I am just trying to see if having a new function name Compute2 does not create more confusion.
There was a problem hiding this comment.
I think it could be confusing that for 4 arguments I'd need to pass a function that takes 4 arguments and for 5 arguments I'd need to pass a function that takes one argument (a list with 5 elements). From that perspective having a separate function, like Compute2, makes sense to me since I don't see a way to distinguish these two cases inside Compute. That said, I think Compute2 could use a better name.
There was a problem hiding this comment.
Actually any function that special cases for smaller number of arguments will do that, say, for example, have overloaded functions for 1, 2, 3 and 4 arguments, and another overload that takes a list (for args > 4). That is pretty normal IMO. Having a different method is more confusing for me.
But if you both prefer this approach, I don't have a problem.
There was a problem hiding this comment.
Mm, I'd like to have the overloaded function here too, but I'm not sure it's possible because of the fact that we're using py::function here and that's what in theory should be used to resolve the overload. In other words, we cannot distinguish when a user passes a function that expects a list and a function that expects 4 parameters. And since we can not distinguish that, we cannot resolve the overload, and hence we need to have explicitly different functions for these two cases. Am I missing something? I just don't see how this would work otherwise: if I provide a function that accepts a list and then happen to pass a list with 2 elements, then the current implementation will try to expand the list into two parameters and call the function with two separate parameters, which will not work.
There was a problem hiding this comment.
Oh, yeah, okay. I see the problem with this approach now. Thanks for clarifying.
| py::return_value_policy::reference); | ||
|
|
||
| te.def( | ||
| "Compute2", |
There was a problem hiding this comment.
Oh, yeah, okay. I see the problem with this approach now. Thanks for clarifying.
Differential Revision: [D28854806](https://our.internmc.facebook.com/intern/diff/D28854806) [ghstack-poisoned]
|
@huiguoo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Pull Request resolved: pytorch#59350 Test Plan: Imported from OSS Reviewed By: mruberry Differential Revision: D28854806 Pulled By: huiguoo fbshipit-source-id: b9091f9183249257aedc1eafb1992e0faf5dea82
4 participants
Stack from ghstack:
Differential Revision: D28854806