inductor cpp wrapper: add GIL release and acquire #111888

chunyuan-w · 2023-10-24T03:08:40Z

Stack from ghstack (oldest at bottom):

-> inductor cpp wrapper: add GIL release and acquire #111888

Support multiple instances inference (in different threads of the same process) as in #93524 (comment).

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @aakhundov @ColinPeppler

[ghstack-poisoned]

pytorch-bot · 2023-10-24T03:08:43Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/111888

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit c969590 with merge base 69b9e54 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

ghstack-source-id: e2ee8aa007141ecc109676e177e2ea6984293103 Pull Request resolved: #111888

Support multiple instances inference (in different threads of the same process) as in #93524. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

ghstack-source-id: 392ca920a7c96eaafd74e323ccd1d8de8ee86929 Pull Request resolved: #111888

jgong5 · 2023-10-24T12:03:53Z

torch/_inductor/codegen/wrapper.py

@@ -1427,6 +1433,7 @@ def generate_return(self, output_refs):
                        + f"new at::Tensor({output}));"
                    )
        else:
+            self.wrapper_call.writeline("py::gil_scoped_acquire acquire;")


with scoped release at the beginning of the function, you don't need to explicitly acquire the gil here, right?

Removed the explicit acquire here.

Support multiple instances inference (in different threads of the same process) as in #93524 (comment). cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

ghstack-source-id: f97e9bcc61df81cc280eb3998216fad4d5bcc55e Pull Request resolved: #111888

desertfire

Test case?

jansel

Is it currently possible for cpp wrapper to call into python code?

Support multiple instances inference (in different threads of the same process) as in #93524 (comment). cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

ghstack-source-id: 41f7b186d5988010f3b8ceec7ffa8a7e84e20bad Pull Request resolved: #111888

jgong5 · 2023-10-30T04:02:41Z

Is it currently possible for cpp wrapper to call into python code?

I'm not aware of a path in cpp wrapper to call into python code? Any concern about it?

chunyuan-w · 2023-10-30T06:48:55Z

Test case?

I added a test case to cover the functionality of a multi-threading scenario.

chunyuan-w · 2023-10-30T23:15:43Z

@pytorchbot rebase

pytorchmergebot · 2023-10-30T23:18:09Z

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

Support multiple instances inference (in different threads of the same process) as in #93524 (comment). cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

pytorchmergebot · 2023-10-30T23:18:26Z

Successfully rebased gh/chunyuan-w/1/orig onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/111888)

ghstack-source-id: ed6c39793b7d9fc578ff7d8ad1442880a49a326a Pull Request resolved: #111888

chunyuan-w · 2023-10-31T03:16:30Z

@pytorchbot merge

pytorchmergebot · 2023-10-31T03:18:21Z

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team

Raised by workflow job

chunyuan-w · 2023-10-31T03:21:14Z

@pytorchbot merge

pytorchmergebot · 2023-10-31T03:23:10Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Support multiple instances inference (in different threads of the same process) as in pytorch#93524 (comment). Pull Request resolved: pytorch#111888 Approved by: https://github.com/jgong5, https://github.com/jansel, https://github.com/desertfire

Fixes #123517. This PR adds the GIL release (originally added in #111888) back. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler amjames desertfire chauhang [ghstack-poisoned]

Fixes #123517. This PR adds the GIL release (originally added in #111888) back following the suggestion here: #123897 (comment). We added a default constructor and an assignment operator for the `RAIIPyObject` class (#123897 (comment)) in order to declare the `custom_op_wrapper` outside of the GIL acquisition scope. Pull Request resolved: #123897 Approved by: https://github.com/peterbell10, https://github.com/jgong5

Fixes pytorch#123517. This PR adds the GIL release (originally added in pytorch#111888) back following the suggestion here: pytorch#123897 (comment). We added a default constructor and an assignment operator for the `RAIIPyObject` class (pytorch#123897 (comment)) in order to declare the `custom_op_wrapper` outside of the GIL acquisition scope. Pull Request resolved: pytorch#123897 Approved by: https://github.com/peterbell10, https://github.com/jgong5

inductor cpp wrapper: add GIL release and acquire

6bbecc7

[ghstack-poisoned]

github-actions bot added module: inductor ciflow/inductor labels Oct 24, 2023

chunyuan-w added a commit that referenced this pull request Oct 24, 2023

inductor cpp wrapper: add GIL release and acquire

19bb91e

ghstack-source-id: e2ee8aa007141ecc109676e177e2ea6984293103 Pull Request resolved: #111888

chunyuan-w marked this pull request as draft October 24, 2023 03:09

pytorchbot added the open source label Oct 24, 2023

chunyuan-w added a commit that referenced this pull request Oct 24, 2023

inductor cpp wrapper: add GIL release and acquire

4a5368b

ghstack-source-id: 392ca920a7c96eaafd74e323ccd1d8de8ee86929 Pull Request resolved: #111888

chunyuan-w marked this pull request as ready for review October 24, 2023 09:28

chunyuan-w requested a review from jgong5 October 24, 2023 09:28

jgong5 requested changes Oct 24, 2023

View reviewed changes

chunyuan-w added a commit that referenced this pull request Oct 25, 2023

inductor cpp wrapper: add GIL release and acquire

682dce2

ghstack-source-id: f97e9bcc61df81cc280eb3998216fad4d5bcc55e Pull Request resolved: #111888

chunyuan-w requested a review from jgong5 October 26, 2023 02:31

jgong5 approved these changes Oct 26, 2023

View reviewed changes

chunyuan-w requested review from jansel and desertfire October 26, 2023 05:08

desertfire reviewed Oct 27, 2023

View reviewed changes

jansel reviewed Oct 27, 2023

View reviewed changes

chunyuan-w added a commit that referenced this pull request Oct 30, 2023

inductor cpp wrapper: add GIL release and acquire

d6740f9

ghstack-source-id: 41f7b186d5988010f3b8ceec7ffa8a7e84e20bad Pull Request resolved: #111888

jansel approved these changes Oct 30, 2023

View reviewed changes

desertfire approved these changes Oct 30, 2023

View reviewed changes

chunyuan-w added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 30, 2023

pytorchmergebot pushed a commit that referenced this pull request Oct 30, 2023

inductor cpp wrapper: add GIL release and acquire

2815962

ghstack-source-id: ed6c39793b7d9fc578ff7d8ad1442880a49a326a Pull Request resolved: #111888

pytorchmergebot added the merging label Oct 31, 2023

pytorchmergebot removed the merging label Oct 31, 2023

chunyuan-w added the topic: not user facing topic category label Oct 31, 2023

pytorchmergebot added the merging label Oct 31, 2023

pytorchmergebot added the Merged label Oct 31, 2023

pytorchmergebot removed the merging label Oct 31, 2023

pytorchmergebot closed this in f50ec34 Oct 31, 2023

facebook-github-bot deleted the gh/chunyuan-w/1/head branch November 3, 2023 14:26

chunyuan-w mentioned this pull request Dec 20, 2023

Test for multiple instances inference #93524

Open

This was referenced Apr 12, 2024

Performance Regression if not release GIL in cpp wrapper #123517

Closed

inductor cpp wrapper: add GIL release back #123897

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inductor cpp wrapper: add GIL release and acquire #111888

inductor cpp wrapper: add GIL release and acquire #111888

chunyuan-w commented Oct 24, 2023 •

edited

pytorch-bot bot commented Oct 24, 2023 •

edited

jgong5 Oct 24, 2023

chunyuan-w Oct 25, 2023

desertfire left a comment

jansel left a comment

jgong5 commented Oct 30, 2023

chunyuan-w commented Oct 30, 2023

chunyuan-w commented Oct 30, 2023

pytorchmergebot commented Oct 30, 2023

pytorchmergebot commented Oct 30, 2023

chunyuan-w commented Oct 31, 2023

pytorchmergebot commented Oct 31, 2023

chunyuan-w commented Oct 31, 2023

pytorchmergebot commented Oct 31, 2023

inductor cpp wrapper: add GIL release and acquire #111888

inductor cpp wrapper: add GIL release and acquire #111888

Conversation

chunyuan-w commented Oct 24, 2023 • edited

pytorch-bot bot commented Oct 24, 2023 • edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/111888

✅ No Failures

jgong5 Oct 24, 2023

Choose a reason for hiding this comment

chunyuan-w Oct 25, 2023

Choose a reason for hiding this comment

desertfire left a comment

Choose a reason for hiding this comment

jansel left a comment

Choose a reason for hiding this comment

jgong5 commented Oct 30, 2023

chunyuan-w commented Oct 30, 2023

chunyuan-w commented Oct 30, 2023

pytorchmergebot commented Oct 30, 2023

pytorchmergebot commented Oct 30, 2023

chunyuan-w commented Oct 31, 2023

pytorchmergebot commented Oct 31, 2023

Merge failed

chunyuan-w commented Oct 31, 2023

pytorchmergebot commented Oct 31, 2023

Merge started

chunyuan-w commented Oct 24, 2023 •

edited

pytorch-bot bot commented Oct 24, 2023 •

edited