-
Notifications
You must be signed in to change notification settings - Fork 26.3k
trace multiple methods #19905
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
trace multiple methods #19905
Conversation
zdevito
left a comment
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.
I think the implementation is correct, but the API is weird at the moment. Not sure users will be able to find trace_dict and it doesn't feel simple. I also think the way def trace is organized can be improved.
torch/jit/__init__.py
Outdated
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.
I am not a fan of this API. I think the problem is that you can't distinguish a dict example input from a dict with string_key->example input. Instead, we can have users pass in the method rather than the field name:
module = torch.jit.trace(n, {n.forward: example_forward_input, n.weight_sum_kernel: example_weight})
torch/jit/__init__.py
Outdated
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.
What is going on here? The method_inputs.items() loop is only sensible if the thing is a module. I feel like this could be refactored to be a lot more readable.
apaszke
left a comment
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.
I also don't like the dict-overloaded API too much. What's the context for this change? It's usually helpful to include some details in the PR description.
|
@apaszke added the description and a simple example. |
zdevito
left a comment
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.
Looks good. Ready to go once check_inputs has a test case.
torch/jit/__init__.py
Outdated
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.
I don't think this line is covered by the tests. check_inputs would need to be different for each method but it looks like we are using the same inputs here.
version2 before cleanup tracing multiple methods
facebook-github-bot
left a comment
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.
@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@Krovatkin merged this pull request in 7ddd5d0. |
This PR adds a new trace API
trace_modulethat will allow us to trace multiple methods as a part of a singleScriptModuleSee the example below.