Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

DeathlessDogface
Copy link

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

import pluggy
import pytest
hookspec = pluggy.HookspecMarker("myproject")
hookimpl = pluggy.HookimplMarker("myproject")


class MySpec:
    """A hook specification namespace."""

    @hookspec
    def myhook(self, arg1, arg2):
        """My special little hook that you can customize."""


class Plugin_1:
    """A hook implementation namespace."""

    @hookimpl
    def myhook(self, arg1, arg2=1):
        print("inside Plugin_1.myhook()")
        return arg1 + arg2


class Plugin_2:
    """A 2nd hook implementation namespace."""

    @hookimpl
    def myhook(self, arg1, arg2=1):
        print("inside Plugin_2.myhook()")
        return arg1 - arg2


def test_case():
    # create a manager and add the spec
    pm = pluggy.PluginManager("myproject")
    pm.add_hookspecs(MySpec)

    # register plugins
    pm.register(Plugin_1())
    pm.register(Plugin_2())

    # call our ``myhook`` hook
    results = pm.hook.myhook(arg1=1, arg2=2)
    assert results == [1 - 2, 1 + 2]

test result

(venv) PS C:\Code\DXN4\demo> pytest .\pluggy_demo2.py         
========================== test session starts ============================
        pm.register(Plugin_1())
        pm.register(Plugin_2())

        # call our ``myhook`` hook
        results = pm.hook.myhook(arg1=1, arg2=2)
>       assert results == [1 - 2, 1 + 2]
E       assert [0, 2] == [-1, 3]
E         At index 0 diff: 0 != -1
E         Use -v to get more diff

pluggy_demo2.py:44: AssertionError
--------------------------------------------------------------------------------------------------------------- Captured stdout call ---------------------------------------------------------------------------------------------------------------- 
inside Plugin_2.myhook()
inside Plugin_1.myhook()
======================== short test summary info ============================= 
FAILED pluggy_demo2.py::test_case - assert [0, 2] == [-1, 3]
========================== 1 failed in 0.15s ================================ 
(venv) PS C:\Code\DXN4\demo>

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a 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 = {
Copy link
Member

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)

Copy link
Author

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.

Copy link
Member

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

Copy link
Author

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 .

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.

@DeathlessDogface
Copy link
Author

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants