-
Notifications
You must be signed in to change notification settings - Fork 5.9k
[SOT] Avoid bind wrong instance on patched method #77227
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
[SOT] Avoid bind wrong instance on patched method #77227
Conversation
|
你的PR提交成功,感谢你对开源项目的贡献! |
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.
Pull request overview
This PR fixes a bug where patched methods (methods attached to instances rather than defined on their class) were incorrectly binding the wrong instance when accessed through the SOT (Symbolic Opcode Translator) system. The fix adds a check to determine if an attribute exists on the class type before binding an instance, preventing incorrect bindings for dynamically-attached methods.
Key changes:
- Added logic to detect patched methods by checking if attributes exist on the class type
- When attributes don't exist on the class, the instance is set to None, allowing the correct instance to be extracted from the method's
__self__ - Applied the fix in both
attr_proxy_getterandload_methodto ensure consistency
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/sot/test_patched_method.py | Adds test case validating that patched methods (methods bound to a different instance) are correctly handled without breaking the graph |
| python/paddle/jit/sot/opcode_translator/executor/variables/base.py | Adds check for whether attribute exists on class type; if not, sets instance to None to avoid binding wrong instance |
| python/paddle/jit/sot/opcode_translator/executor/opcode_executor.py | Adds the same hasattr check when loading methods to ensure consistent handling of patched methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DrRyanHuang
left a comment
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.
最小复现demo真简洁喵 👍
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #77227 +/- ##
===========================================
Coverage ? 100.00%
===========================================
Files ? 2
Lines ? 4
Branches ? 0
===========================================
Hits ? 4
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PR Category
Execute Infrastructure
PR Types
Bug fixes
Description
同 #73038,
paddle.distributed.fleet会有同样的问题,这导致在调用分布式 API 的情况下会报错,本 PR 避免 bind 时候 bind 错 instancePaddle/python/paddle/distributed/fleet/__init__.py
Lines 55 to 114 in b0cdc64
cpython 在
LOAD_METHOD时如果不是obj.__class__.<name>.__get__(obj, obj.__class__)这种形式的情况下,是不会走第一个meth_found分支的对于当前 case,这是因为
obj.__dict__有<name>,即 patched method,这种情况会被认为meth_found=0https://github.com/python/cpython/blob/1173f8068b9a8c1168780532c466c391be10bf67/Objects/object.c#L1174-L1181
另外
super().fn之前也走了错误的路径,但是能跑,tp_getattro实际是super_getattro,所以在Py_TYPE(obj)->tp_getattro != PyObject_GenericGetAttr就会走到meth_found=0https://github.com/python/cpython/blob/1173f8068b9a8c1168780532c466c391be10bf67/Objects/object.c#L1147-L1151