Changes to compile with 3.13 #126033
Changes to compile with 3.13 #126033
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/126033
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 2b2820e with merge base 037615b ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| static Py_tss_t eval_frame_callback_key = Py_tss_NEEDS_INIT; | ||
|
|
||
| inline static PyObject* eval_frame_callback_get(void) { | ||
| void* result = PyThread_tss_get(&eval_frame_callback_key); |
There was a problem hiding this comment.
Does this work? Don't you need to call PyThread_tss_create before any get/set with this API?
I think this will probably be simpler if you just use C11 or platform thread-local definitions:
thread_localif you are compiling this file as C11 (or newer)- Otherwise, do something like https://github.com/python/cpython/blob/7d7eec595a47a5cd67ab420164f0059eb8b9aa28/Include/pyport.h#L488-L494
There was a problem hiding this comment.
This is the current dynamo code that was just moved to be in the right ifdef section. So I guess it works because dynamo is doing that today already? :p
This file is pure C, not C11 because calling cpython code like this from c++ is tricky and simpler to keep this being pure C.
There was a problem hiding this comment.
Ok, that's fine. But, fwiw, C11 is C not C++.
There was a problem hiding this comment.
:o
Sorry misread that.
cc @jansel or @williamwen42 if you have any more context on migrating this to C11 ?
There was a problem hiding this comment.
PyThread_tss_create is called in torch_c_dynamo_eval_frame_init below. I'm not aware of any plans to migrate to C11.
|
@pytorchbot merge |
Merge startedYour 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 |
This is mainly: - Fix refcount access macro - Hide all the Dynamo code that needs update as usual - Add _PyWeakref_ClearRef as an extern provided by CPython. Including the pycore header that defines it would require raw c include shenanigans that I don't think are worth it. This allows to build both with regular and nogil version of cpython. Both Note that this requires the 3.13 branch at least past [d3094744d40de2deefbda9b1996d5029c9ebf0b0](python/cpython@d309474) which we need for mimalloc include and weakref function being exposed. debug-only issues in pybind11 with PyMem_MALLOC vs PyObject_MALLOC being should be synced either by updating pybind or cpython. @colesbury I can send a PR to ifdef the proper use in pybind if you think that this is the best solution here? Pull Request resolved: pytorch#126033 Approved by: https://github.com/colesbury
This is mainly:
This allows to build both with regular and nogil version of cpython. Both
Note that this requires the 3.13 branch at least past d3094744d40de2deefbda9b1996d5029c9ebf0b0 which we need for mimalloc include and weakref function being exposed.
debug-only issues in pybind11 with PyMem_MALLOC vs PyObject_MALLOC being should be synced either by updating pybind or cpython. @colesbury I can send a PR to ifdef the proper use in pybind if you think that this is the best solution here?
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang