Skip to content

Merge CompilationUnit from torch._C and torch.jit#50614

Closed
t-vi wants to merge 3 commits intopytorch:masterfrom
t-vi:python-jit-compilationunit
Closed

Merge CompilationUnit from torch._C and torch.jit#50614
t-vi wants to merge 3 commits intopytorch:masterfrom
t-vi:python-jit-compilationunit

Conversation

@t-vi
Copy link
Copy Markdown
Collaborator

@t-vi t-vi commented Jan 15, 2021

This simplifies our handling and allows passing CompilationUnits from Python to C++ defined functions via PyBind easily.

Discussed on Slack with @SplitInfinity

@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Jan 15, 2021
@t-vi t-vi force-pushed the python-jit-compilationunit branch from 7f7f2c7 to c4f41d4 Compare January 15, 2021 21:23
@t-vi t-vi force-pushed the python-jit-compilationunit branch from c4f41d4 to 023380e Compare January 15, 2021 22:48
@t-vi
Copy link
Copy Markdown
Collaborator Author

t-vi commented Jan 16, 2021

Forgot the signatures. Will fix.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 16, 2021

Codecov Report

Merging #50614 (156a3bd) into master (a469336) will increase coverage by 0.00%.
The diff coverage is 95.23%.

@@           Coverage Diff           @@
##           master   #50614   +/-   ##
=======================================
  Coverage   80.65%   80.65%           
=======================================
  Files        1912     1912           
  Lines      208048   208071   +23     
=======================================
+ Hits       167810   167829   +19     
- Misses      40238    40242    +4

@t-vi t-vi requested a review from SplitInfinity January 16, 2021 17:52
@t-vi
Copy link
Copy Markdown
Collaborator Author

t-vi commented Jan 16, 2021

I think this is ready.

SplitInfinity
SplitInfinity reviewed Jan 16, 2021
View reviewed changes
Copy link
Copy Markdown

@SplitInfinity SplitInfinity left a comment

Choose a reason for hiding this comment

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

Nice! I left a few comments about things I'd like to discuss.

Comment thread test/jit/test_python_bindings.py Outdated
Comment thread torch/jit/_script.py Outdated
Comment thread torch/csrc/jit/python/script_init.cpp Outdated
Comment thread torch/csrc/jit/python/script_init.cpp Outdated
Comment thread torch/csrc/jit/python/script_init.cpp Outdated
Comment thread torch/csrc/jit/python/script_init.cpp Outdated
Comment thread torch/csrc/jit/python/script_init.cpp Outdated
Comment thread torch/_C/__init__.pyi.in Outdated
Comment thread test/jit/test_python_bindings.py Outdated
Comment thread test/jit/test_python_bindings.py Outdated
@SplitInfinity
Copy link
Copy Markdown

SplitInfinity commented Jan 16, 2021

@t-vi Can you fix up the two small issues (the name of the new function you added and the binding for getting the python CU)? I can approve once that's done. In the meantime, I will import this to run internal FB tests.

facebook-github-bot
facebook-github-bot reviewed Jan 16, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

SplitInfinity
SplitInfinity reviewed Jan 16, 2021
View reviewed changes
Comment thread torch/csrc/jit/python/script_init.cpp Outdated
Copy link
Copy Markdown

@SplitInfinity SplitInfinity Jan 16, 2021

Choose a reason for hiding this comment

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

One more thing - why is this module_ instead of module?

Copy link
Copy Markdown
Collaborator Author

@t-vi t-vi Jan 17, 2021

Choose a reason for hiding this comment

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

Sleeping on the _....

Copy link
Copy Markdown
Collaborator Author

@t-vi t-vi Jan 17, 2021

Choose a reason for hiding this comment

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

Thank you for catching all these!

@t-vi
Copy link
Copy Markdown
Collaborator Author

t-vi commented Jan 17, 2021 •
edited
Loading

Something has gone wrong and the previous image isn't available for the merge-base of your branch contact the PyTorch team to restore the original images

Notably it doesn't say "try rebasing"...

@ngimel ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 18, 2021
facebook-github-bot
facebook-github-bot reviewed Jan 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@SplitInfinity
Copy link
Copy Markdown

SplitInfinity commented Jan 19, 2021

Something has gone wrong and the previous image isn't available for the merge-base of your branch contact the PyTorch team to restore the original images

Notably it doesn't say "try rebasing"...

It does not, but that's the recommended solution (not mentioned because this is not "supposed to happen").

@t-vi
Copy link
Copy Markdown
Collaborator Author

t-vi commented Jan 23, 2021

@SplitInfinity can I do anything to help it along?

@SplitInfinity
Copy link
Copy Markdown

SplitInfinity commented Jan 25, 2021 •
edited
Loading

I encountered some flaky internal tests so I was rebasing for a few days to make sure this PR wasn't the issue. Everything looks okay now, so I'm going to try and merge this today.

t-vi added 3 commits January 25, 2021 19:53
@t-vi
Merge CompilationUnit from torch._C and torch.jit
e1164b7 
This simplifies our handling and allows passing CompilationUnits
from Python to C++ defined functions via PyBind easily.
@t-vi
fix type hints
fb0c60f
@t-vi
Improvements from review. Thank you!
695cbff
@t-vi t-vi force-pushed the python-jit-compilationunit branch from d9e56eb to 695cbff Compare January 25, 2021 18:53
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jan 25, 2021

@SplitInfinity merged this pull request in ac0a3cc.

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@t-vi @facebook-github-bot
Merge CompilationUnit from torch._C and torch.jit (pytorch#50614)
64a7b31 
Summary:
This simplifies our handling and allows passing CompilationUnits from Python to C++ defined functions via PyBind easily.

Discussed on Slack with SplitInfinity

Pull Request resolved: pytorch#50614

Reviewed By: anjali411

Differential Revision: D25938005

Pulled By: SplitInfinity

fbshipit-source-id: 94aadf0c063ddfef7ca9ea17bfa998d8e7b367ad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@SplitInfinity SplitInfinity SplitInfinity left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@t-vi @SplitInfinity @facebook-github-bot @ngimel @pytorchbot