-
Notifications
You must be signed in to change notification settings - Fork 7k
[core] Fix python 3.12 asyncio RecursionError leading to objects_valid check #57253
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
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -679,25 +679,55 @@ def compute_task_id(ObjectRef object_ref): | |
|
|
||
|
|
||
| cdef increase_recursion_limit(): | ||
| """Double the recusion limit if current depth is close to the limit""" | ||
| """ | ||
| Ray does some weird things with asio fibers and asyncio to run asyncio actors. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Killing boost fiber is on the @edoakes's wishlist
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ya ik I talked to him about it, he said he's gonna try prioritizing next quarter 👀 |
||
| This results in the Python interpreter thinking there's a lot of recursion depth, | ||
| so we need to increase the limit when we start getting close. | ||
|
|
||
| 0x30C0000 is Python 3.12 | ||
dayshah marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| On 3.12, when recursion depth increases, c_recursion_remaining will decrease | ||
| and this is what's acutally compared to raise a RecursionError. So increasing | ||
| it by 1000 when it drops below 1000 will keep us from raising the RecursionError. | ||
| DoOrGetRecursionMadness will return (false, 0), so we don't set with Py_SetRecursionLimit. | ||
| 0x30B00A4 is Python 3.11 | ||
| On 3.11, the recursion depth can be calculated with recursion_limit - recursion_remaining. | ||
| We can get the current limit with Py_GetRecursionLimit and set it with Py_SetRecursionLimit. | ||
| DoOrGetRecursionMadness will return (true, current_depth) | ||
| On older versions | ||
| There's simply a recursion_depth variable and we can increase the max the same | ||
| way we do for 3.11. | ||
| DoOrGetRecursionMadness will return (true, current_depth) | ||
| """ | ||
| cdef: | ||
| CPyThreadState * s = <CPyThreadState *> PyThreadState_Get() | ||
| int current_limit = Py_GetRecursionLimit() | ||
| int new_limit = current_limit * 2 | ||
| int new_limit = current_limit * 2 # Only for versions less than 3.12 | ||
| cdef extern from *: | ||
| """ | ||
| #if PY_VERSION_HEX >= 0x30C0000 | ||
| #define CURRENT_DEPTH(x) ((x)->py_recursion_limit - (x)->py_recursion_remaining) | ||
| std::pair<bool, int> DoOrGetRecursionMadness(PyThreadState *x) { | ||
|
||
| if (x->c_recursion_remaining < 1000) { | ||
| x->c_recursion_remaining += 1000; | ||
| } | ||
| return std::make_pair(false, 0); | ||
| } | ||
| #elif PY_VERSION_HEX >= 0x30B00A4 | ||
| #define CURRENT_DEPTH(x) ((x)->recursion_limit - (x)->recursion_remaining) | ||
| std::pair<bool, int> DoOrGetRecursionMadness(PyThreadState *x) { | ||
| return std::make_pair(true, x->recursion_limit - x->recursion_remaining); | ||
| } | ||
edoakes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #else | ||
| #define CURRENT_DEPTH(x) ((x)->recursion_depth) | ||
| std::pair<bool, int> DoOrGetRecursionMadness(PyThreadState *x) { | ||
| return std::make_pair(true, x->recursion_depth); | ||
| } | ||
| #endif | ||
| """ | ||
| int CURRENT_DEPTH(CPyThreadState *x) | ||
| c_pair[c_bool, int] DoOrGetRecursionMadness(CPyThreadState *x) | ||
|
|
||
| c_pair[c_bool, int] result = DoOrGetRecursionMadness(s) | ||
| c_bool need_to_increase = result.first | ||
| int current_depth = result.second | ||
|
|
||
| int current_depth = CURRENT_DEPTH(s) | ||
| if current_limit - current_depth < 500: | ||
| if need_to_increase and current_limit - current_depth < 500: | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Py_SetRecursionLimit(new_limit) | ||
| logger.debug("Increasing Python recursion limit to {} " | ||
| "current recursion depth is {}.".format( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field exists in all Python versions?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it doesn't but having it here doesn't cause any issues even if used with old python versions
recursion_limitandrecursion_remainingalso don't exist for all python versionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, interesting