Repository navigation
refactor: remove the unused Sockets.__getattribute__ override - #12966
bhaskargurram-ai wants to merge 3 commits into
Conversation
__getattribute__ looked up a `_sockets` attribute, but the one __init__ assigns is `_sockets_dict`. The lookup raised AttributeError on every attribute access and fell through to object.__getattribute__, so the intended fast path never ran. Socket access still worked, but only through the copy __init__ places in __dict__ -- which made the dead branch look load-bearing and put an exception on the path of every attribute access. Reading the right name makes the branch live: 200k socket attribute accesses drop from 0.125s to 0.031s. External behaviour is unchanged, since the __dict__ copy already shadowed class attributes for the same names. Fixes deepset-ai#12939
|
@bhaskargurram-ai is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
Hi @bhaskargurram-ai, thanks a lot for your contribution! 🙏 We noticed that the Contributor License Agreement (CLA) check ( To get your PR reviewed, please sign the CLA via the link in the |
HaystackBot
added
the
cla-pending
HaystackBot
removed
the
cla-pending
|
Thanks for signing the CLA, @bhaskargurram-ai! 🎉 This PR is now ready for review again and the reviewer has been re-assigned. |
|
Hi @julian-risch, gentle ping on this one when you have a moment. Sockets.getattribute looked up _sockets instead of _sockets_dict, so the fast path never ran: 200k socket attribute accesses take 0.031 s with the fix vs 0.125 s before. The CLA is signed. Thanks! |
github-actions
Bot
added
the
P2
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
| @@ -125,7 +125,7 @@ def _component_name(self) -> str: | |||
|
|
|||
| def __getattribute__(self, name: Any) -> Any: | |||
There was a problem hiding this comment.
This override has never run since it was added in #6916 because it looked up _sockets, and socket access has always gone through the copy __init__ and __setitem__ put in __dict__. Could we remove the __getattribute__ override instead? I tried that locally: test/core/component and test/core/pipeline pass, apart from the new test that deletes from io.__dict__, and socket lookup is about 3x faster than with this change. WDYT?
|
|
||
| assert io.input_1 == comp.__haystack_input__._sockets_dict["input_1"] # type: ignore[attr-defined] | ||
|
|
||
| def test_getattribute_does_not_shadow_methods_or_private_attributes(self): |
There was a problem hiding this comment.
This test passes on main as well since no socket name collides with get or _sockets_dict. Lets remove it.
The override never returned a socket (see the previous commit), and socket attribute access already goes through the __dict__ copies made by __init__ and __setitem__. Removing it, as suggested in review, makes socket attribute access about 14x faster than on main (728 ns -> 51 ns per access in a micro-benchmark). Drop the two tests that only exercised the override.
bhaskargurram-ai
changed the title
github-actions
Bot
removed
the
P2
This branch has not been deployed
4 participants
Proposed Changes:
Sockets.__getattribute__looked up a_socketsattribute that no instance has, so it raised and caughtAttributeErroron every attribute access and never returned a socket. Socket access already goes through the copies__init__and__setitem__put in__dict__. As suggested in review, this removes the override instead of fixing it.How did you test it?
test/core/componentandtest/core/pipelinepass (the only failures are missing optional deps and fail identically on main). Micro-benchmark ofcomponent.__haystack_input__.<socket>: 728 ns on main, 51 ns with this change.