Fix profiler on cpython-3.13 #153848
Fix profiler on cpython-3.13 #153848
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/153848
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 48 PendingAs of commit 507e70e with merge base ae0e8f0 ( UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Per [PEP 667](https://peps.python.org/pep-0667/) `PyFrame_GetLocals` no longer returns dict, but rather instance of `PyFrameLocalsProxy_Type`, so calling `PyDict_GetItemString` is no longer valid and must be replaced with `PyMapping_GetItemString` Fixes #148273 ghstack-source-id: a6f5b55 Pull Request resolved: #153848
|
@pytorchbot merge -f "It was green in the previous iteration, this just fixes the linter" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
As underlying issue were fixed by #153848 Fixes #142166 Pull Request resolved: #153857 Approved by: https://github.com/williamwen42 ghstack dependencies: #153848
| @@ -882,15 +882,26 @@ void PythonTracer::recordPyCall( | |||
| // `PyFrame_FastToLocals` which forces the interpreter to materialize | |||
| // the full dict of locals. | |||
| auto locals = THPObjectPtr(PyFrame_GetLocals(frame)); | |||
|
|
|||
| #if PY_MAJOR_VERSION < 3 || (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION < 13) | |||
There was a problem hiding this comment.
we have #if IS_PYTHON_3_13_PLUS and opposite as a shorthand for these !
| #else | ||
| // In Python-3.13+ `PyFrame_GetLocals()` returns instance of | ||
| // PyFrameLocalsProxy_Type See PEP 667 for more info | ||
| auto self = THPObjectPtr(PyMapping_GetItemString(locals, "self")); |
There was a problem hiding this comment.
This is a mapping for all versions of cpython (since dict are mapping), should we not just use the mapping API for all versions to keep this simpler?
|
@pytorchbot cherry-pick --onto release/2.7 --category regression |
|
❌ 🤖 pytorchbot command failed: Try |
|
@pytorchbot cherry-pick --onto release/2.7 -c regression |
Per [PEP 667](https://peps.python.org/pep-0667/) `PyFrame_GetLocals` no longer returns dict, but rather instance of `PyFrameLocalsProxy_Type`, so calling `PyDict_GetItemString` is no longer valid(it will always return None) and must be replaced with `PyMapping_GetItemString` Tested by partially reverting #141674 full revert will be done in the followup PR Fixes #148273 Pull Request resolved: #153848 Approved by: https://github.com/Skylion007 (cherry picked from commit c0343b1)
Cherry picking #153848The cherry pick PR is at #154037 and it is recommended to link a regression cherry pick PR with an issue. The following tracker issues are updated: Details for Dev Infra teamRaised by workflow job |
Fix profiler on cpython-3.13 (#153848) Per [PEP 667](https://peps.python.org/pep-0667/) `PyFrame_GetLocals` no longer returns dict, but rather instance of `PyFrameLocalsProxy_Type`, so calling `PyDict_GetItemString` is no longer valid(it will always return None) and must be replaced with `PyMapping_GetItemString` Tested by partially reverting #141674 full revert will be done in the followup PR Fixes #148273 Pull Request resolved: #153848 Approved by: https://github.com/Skylion007 (cherry picked from commit c0343b1) Co-authored-by: Nikita Shulga <nshulga@meta.com>
Stack from ghstack (oldest at bottom):
Per PEP 667
PyFrame_GetLocalsno longer returns dict, but rather instance ofPyFrameLocalsProxy_Type, so callingPyDict_GetItemStringis no longer valid(it will always return None) and must be replaced withPyMapping_GetItemStringTested by partially reverting #141674 full revert will be done in the followup PR
Fixes #148273