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.

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

Infer types for arguments of methods not invoked directly by monkeytype #57202

Closed
wants to merge 14 commits into from

Conversation

nikithamalgifb
Copy link
Contributor

@nikithamalgifb nikithamalgifb commented Apr 28, 2021

Support adding type annotations for class methods and nn.Module methods which are not invoked under the hood of MonkeyType

** Changes **

  • This PR involves a slight change in how the example inputs are passed while scripting class and nn.Module objects.
  • The example inputs passed to _script_pdt is of the following format:
    • example_inputs= [(obj.method1, (arg_list)), (obj.method2, (arg_list)),]
  • For nn.Modules, to infer types for forward methods, example_inputs can be passed in two ways:
    • example_inputs= [(obj.forward, (arg_list, ))]
    • example_inputs = [(obj, (arg_list, ) )]

@facebook-github-bot facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Apr 28, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 28, 2021

💊 CI failures summary and remediations

As of commit 7d826ba (more details on the Dr. CI page):


None of the CI failures appear to be your fault 💚



🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

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.

Click here to manually regenerate this comment.

@nikithamalgifb nikithamalgifb force-pushed the infer_types_methods branch 2 times, most recently from e292056 to 4e4fd4e Compare April 28, 2021 22:12
@codecov
Copy link

codecov bot commented Apr 29, 2021

Codecov Report

Merging #57202 (ded32c5) into master (2a178d3) will increase coverage by 20.22%.
The diff coverage is 0.00%.

❗ Current head ded32c5 differs from pull request most recent head 98015ba. Consider uploading reports for the commit 98015ba to get more accurate results

@@             Coverage Diff             @@
##           master   #57202       +/-   ##
===========================================
+ Coverage   57.46%   77.68%   +20.22%     
===========================================
  Files         596     1954     +1358     
  Lines       76343   194572   +118229     
===========================================
+ Hits        43872   151160   +107288     
- Misses      32471    43412    +10941     

@nikithamalgifb
Copy link
Contributor Author

nikithamalgifb commented Apr 29, 2021

Note: There are some redundancies(test helpers) with this PR as seen in #57165. They should go away once #57165 lands and the PR is rebased.

torch/jit/_script.py Outdated Show resolved Hide resolved
torch/jit/_script.py Outdated Show resolved Hide resolved
torch/jit/_script.py Outdated Show resolved Hide resolved
torch/jit/_script.py Outdated Show resolved Hide resolved
@@ -851,7 +851,8 @@ def call_prepare_scriptable_func(obj):
memo: Dict[int, torch.nn.Module] = {}
return call_prepare_scriptable_func_impl(obj, memo)

def _script_pdt(obj, optimize=None, _frames_up=0, _rcb=None, example_inputs: Optional[List[Tuple]] = None):
def _script_pdt(obj, optimize=None, _frames_up=0, _rcb=None,
example_inputs: Union[List[Tuple], Dict[Callable, List[Tuple]], None] = None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a task in asana for you about creating a docstring for _script_pdt since its usage is sort of complex now.

Comment on lines +98 to +99
scripted_pdt_model = torch.jit._script_pdt(outer_pdt_model, example_inputs={inner_pdt_model: inner_input,
outer_pdt_model: outer_input, })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inner_pdt_model: inner_input should not be needed here. Thus we should be able to pass in outer_input in simple format of List[Tuple]. Simple format input can be used in following few test cases as well.

@facebook-github-bot
Copy link
Contributor

@nikithamalgifb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@nikithamalgifb merged this pull request in 9063cb0.

krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
…pe (pytorch#57202)

Summary:
Support adding type annotations for class methods and nn.Module methods which are not invoked under the hood of MonkeyType

** Changes **
* This PR involves a slight change in how the example inputs are passed while scripting `class` and `nn.Module` objects.
* The example inputs passed to `_script_pdt` is of the following format:
     - example_inputs= [(obj.method1, (arg_list)), (obj.method2, (arg_list)),]
* For nn.Modules, to infer types for `forward` methods, example_inputs can be passed in two ways:
    - example_inputs= [(obj.forward, (arg_list, ))]
    - example_inputs = [(obj, (arg_list, ) )]

Pull Request resolved: pytorch#57202

Reviewed By: desertfire

Differential Revision: D28382827

Pulled By: nikithamalgifb

fbshipit-source-id: 5481467f3e909493bf3f439ee312056943508534
@github-actions github-actions bot deleted the infer_types_methods branch February 11, 2024 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants