-
Notifications
You must be signed in to change notification settings - Fork 125
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
Fix the issue where passing keyword argumentss was not effective #475
base: main
Are you sure you want to change the base?
Conversation
… enhanced the test cases for the _multicall() function
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
I'm torn on this one, as we have a pending design/implementation action for specifically choosen default values (for backward/forward compatibility of hook impl's/specs as parameters are added/removed
part of me wants to formally forbid default values other than None so they can specifically be used to manage addition/removal of parameters in specs/impls
@@ -70,6 +69,11 @@ def _multicall( | |||
for hook_impl in reversed(hook_impls): | |||
try: | |||
args = [caller_kwargs[argname] for argname in hook_impl.argnames] | |||
kwargs = { |
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.
note that adding this bit undoes a specifically chosen optimization (that we might want to reevaluate later)
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.
Thank you for pointing this out. I understand what you mean. However, the current design has a major limitation that prevents the passing of keyword arguments.
Could you please elaborate on the specific optimization that is being undone by my addition? I would like to understand its purpose and how it was beneficial to the project.
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.
initially pluggy used kwarg passing
i'm not quite sure how long ago, but eventually we determined that with extracting only positional arguments, things where much faster (i vaguely recall 2x improvement back in the time)
if we now add kwargs back, that effect is likely undone
as recent python versions have optimizations for calling, i believe a new measurement makes sense
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.
Thank you for sharing this information. Could you please elaborate on the performance evaluation method used? I would like to measure the performance based on the new version of Python to have a more accurate comparison. It would be helpful if you could provide more details on the testing environment .
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.
Hi Ronny,
I also got the same requirement as well.
Any plan to run a new measurement? It is appreciated if anything I can help.
I'm sorry, I just saw this reply. Now I understand the comments in the code, thank you. However, I would still like to recommend my changes to you again. |
Fix the issue where passing keyword argumentss was not effective, and enhanced the test cases for the _multicall() function
The issue like this:
test code
test result