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

[dynamo][eval_frame] Set destroy_extra_state deleter as part of co_extra #107117

Closed
wants to merge 6 commits into from

Conversation

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 14, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit a26c3a1 with merge base aa04b05 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@ezyang
Copy link
Contributor

ezyang commented Aug 14, 2023

letting @jansel on this one

@@ -420,7 +425,6 @@ inline static void set_extra_state(PyCodeObject* code, ExtraState* extra_state)
// set_extra_state responsibility to clean it up. It will be deleled during
// the reset_code/skip, when the set_extra_state is called with
// NULL/SKIP_CODE.
destroy_extra_state(code);
_PyCode_SetExtra((PyObject*)code, extra_index, extra_state);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming _PyCode_SetExtra calls destroy_extra_state if the value is already set?

(I have not confirmed this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does

image

…rt of co_extra"



Using the `freefunc` facility to free the ExtraState objects - https://peps.python.org/pep-0523/

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 aakhundov

[ghstack-poisoned]
…rt of co_extra"



Using the `freefunc` facility to free the ExtraState objects - https://peps.python.org/pep-0523/

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 aakhundov

[ghstack-poisoned]
…rt of co_extra"



Using the `freefunc` facility to free the ExtraState objects - https://peps.python.org/pep-0523/

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 aakhundov

[ghstack-poisoned]
anijain2305 added a commit that referenced this pull request Aug 17, 2023
ghstack-source-id: e6a38392b88e31d320ef4ff1a7163f7218715d91
Pull Request resolved: #107117
…rt of co_extra"



Using the `freefunc` facility to free the ExtraState objects - https://peps.python.org/pep-0523/

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 aakhundov

[ghstack-poisoned]
…rt of co_extra"



Using the `freefunc` facility to free the ExtraState objects - https://peps.python.org/pep-0523/

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 aakhundov

[ghstack-poisoned]
@anijain2305
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Aug 17, 2023
@pytorchmergebot
Copy link
Collaborator

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

@anijain2305
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

@facebook-github-bot facebook-github-bot deleted the gh/anijain2305/107/head branch August 21, 2023 14:16
pytorchmergebot pushed a commit that referenced this pull request Aug 21, 2023
This PR makes CacheEntry a PyObject. This is prep PR for cache size changes. As CacheEntry is a py object, we can now traverse the linked list in Python and write cache size policies. It was possible to do in C, but Python is just easier to iterate upon. We call convert_frame only when we (re)compile, so a small bump in latency going from C to Python is acceptable here.

Pull Request resolved: #107405
Approved by: https://github.com/ezyang
ghstack dependencies: #106917, #107117
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants