support defaults to enable deprecation

[RonnyPfannschmidt]

this is to help pytest-dev/pytest#1372

the idea is, that if a hook is invoked with a missing, but specced argument, then the default is used instead

so if a hook caller would call hook.pytest_deselected(items=...)' and the hook was defined withdef pytest_deselected(items, reason='No Reason Given)`
then the receivers would receive the defined default reason

in order to support deprecation of such usage we could later devise a mark for deprecated defaults

[hpk42]

Usually a project defining a hookspec is also calling it. So this is basically a very minor convenience way of expressing a default and it blurs the line between what is a spec and what is implementation. I think it's clearer to request from a hook calling site to provide all arguments and not have to worry cross-checking with the spec on what parameters are actually passed to impls.

[RonnyPfannschmidt]

@hpk - this is directly broken for how we document pytest_deselected

atm its missing a actually sane way to transfer the reason

[hpk42]

What exactly is broken? What is not working as expected? At the moment we don't promise that we do anything with default arguments in specs. They are currently ignored -- we could throw an error to be more explicit. I hold providing call-kwargs values through a hookspec is confusing.

[RonnyPfannschmidt]

@hpk42 the underlying problem is the following

pytest_deselected(items) is documented to be called as hook

if we now want to add a reason argument so it becomes pytest_deselected(items, reason) - all external users break

[hpk42]

ah, now i get it, thanks. So this is a situation where callers of a hook live in separate projects and thus changing the spec becomes hard. This kind of trumps (!) my concern then i guess. Feel free to submit a PR. if you hurry it might make it into pluggy-0.5.

[goodboy]

@hpk42 pretty sure I can nail out this one :)

[hpk42]

On Fri, Nov 11, 2016 at 07:04 -0800, goodboy wrote:

@hpk42 pretty sure I can nail out this one :)

@nicoddemus reminded me that pytest actually has its own vendored version of pluggy. If that is true also for older pytest versions (i think it is) we don't actually need to support multicall neccessarily anymore. It might mean, though, that pytest only wants to go for vendoring a non-multicall pluggy for pytest-4.0. Which in turns means we might want to support it for a while still at best through an opt-in parameter to instantiating a PluginManager.

[RonnyPfannschmidt]

@tgoodlet this one might be a bit tricky

[goodboy]

@RonnyPfannschmidt just to make sure I understand this correctly:

hookspec = HookspecMarker("prefix")
hookimpl = HookimplMarker("prefix")

class Spec:
    @hookspec
    def prefix_myhook(self, doggy, kitty='mean'):
        pass

@hookimpl
def prefix_myhook(doggy, kitty):
    assert doggy is not 'smelly', "Kick out the dog"
    print("shocker kitty is always {}".format(kitty))


@hookimpl
def prefix_myhook(doggy, kitty='smelly'):
    assert doggy is not 'smelly', "Kick out the dog"
    print("shocker kitty is always {}".format(kitty))

pm = PluginManger('prefix')
pm.add_hookspec(Spec)
pm.register(sys.modules[__name__])

• precedence is given to the spec's default arg unless overridden in the implementation
• passing the arg explicitly in the hook call overrides both?

>>> pm.hook.prefix_myhook(doggy='just bathed')
"shocker kitty is always mean"  # from the first impl
"shocker kitty is always smelly"  # from the second impl

>>> pm.hook.prefix_myhook(doggy='just bathed', kitty='grumpy')
"shocker kitty is always grumpy"  # from the first impl
"shocker kitty is always grumpy"  # from the second impl

[hpk42]

uh, that precedence questions highlights that allowing default args in specs is tricky. if anything, i'd argue that the impl's default should take precedence over the spec's default.

I am afraid however we decide about precedence, it's going to be surprising to some and you have to check three places to understand where a value comes from ... sorry, ronny, i am getting unsure again if this is the right approach to solve the underlying problem we discussed earlier: callers of a hook become incompatible if an argument is added to the spec. I'd now like to get others to comment on the problem before you tackle impl, tests, docs.

My view is that pluggy is foremost about 1:N calls, not sure much about M:N calls (with M = numb of the different places where a call is made).

On a side note, maybe this pytest deselect hook calling is a bit of a bad idea anyway. pytest's core collection hook should automatically call it by looking at pre-post list of items. Or it should even all be just done by the terminal reporter. This way user code is not concerned with it at all.

[nicoddemus]

uh, that precedence questions highlights that allowing default args in specs is tricky. if anything, i'd argue that the impl's default should take precedence over the spec's default.

My initial reaction was the opposite, which only emphasizes your point about this being tricky.

maybe this pytest deselect hook calling is a bit of a bad idea anyway.

FWIW, I always thought it a bit odd... it felt weird calling a core hook from a plugin implementation.

Or it should even all be just done by the terminal reporter.

The same thought occurred to me when I had to call pytest_deselected in a plugin once. I thought at the time the terminal writer could find out which tests were deselected automatically by writing a hookwrapper around pytest_collection_modifyitems.

Do we have other hooks with the same behavior as pytest_deselected(items)? Perhaps @RonnyPfannschmidt has other examples from custom plugins that have the same behavior.

If there's no other cases, we might instead consider just getting rid of the odd child that is pytest_deselected.

[goodboy]

I am afraid however we decide about precedence, it's going to be surprising to some and you have to check three places to understand where a value comes from

@hpk42 just want to clarify the 3 places in reverse precedence order - spec, impl, call.

I guess your concern is with debugging? As far as I understand the implementer of a hook shouldn't be too concerned about where a value came from.

callers of a hook become incompatible if an argument is added to the spec.

@hpk42 I'm not sure this is true as long as the new argument is a kwarg. In fact that's usually the beauty of one.

As an example I already have a working draft patch of _MultiCall.execute() which looks as follows:

     def execute(self):
-        all_kwargs = self.kwargs
+        all_kwargs = copy.copy(self.kwargs)
         self.results = results = []
         firstresult = self.specopts.get("firstresult")

         while self.hook_impls:
             hook_impl = self.hook_impls.pop()
+            if "__multicall__" in hook_impl.argnames:
+                all_kwargs["__multicall__"] = self
             try:
-                args = [all_kwargs[argname] for argname in hook_impl.argnames]
+                args = [all_kwargs.pop(argname) for argname in hook_impl.argnames]
             except KeyError:
                 for argname in hook_impl.argnames:
-                    if argname not in all_kwargs:
+                    if argname not in self.kwargs:
                         raise HookCallError(
                             "hook call must provide argument %r" % (argname,))
             if hook_impl.hookwrapper:
-                return _wrapped_call(hook_impl.function(*args), self.execute)
-            res = hook_impl.function(*args)
+                return _wrapped_call(hook_impl.function(*args, **all_kwargs),
+                                     self.execute)
+            res = hook_impl.function(*args, **all_kwargs)
             if res is not None:
                 if firstresult:
                     return res

My view is that pluggy is foremost about 1:N calls, not sure much about M:N calls (with M = numb of the different places where a call is made).

Fwiw M calls turns out to be very useful for event like systems made with pluggy. I'm actually using this for a project where we signal to plugins the dynamic setup/teardown of network resources. This allows plugins to do things like log capture and tracing on remote hosts that are provided by elastic VM and container systems.

[RonnyPfannschmidt]

my current oppinion is that spec defaults should always win over impl defaults

a hookimpl should be able to declare the default for backward compatibility (say a hook adds a new parameter you want to use if availiable, but still support older versions of the program that did not support/supply it

@hpk42 the items object for the modifyitems hook could have a richer api than just being a normal list
one thing is certain, terminalwriter is unable to figure a reason ^^

[hpk42]

@tgoodlet if i write an impl like def myhook(arg1, arg2=20) i kind of expect that when someone calls myhook(arg1=10) to see arg2=20 passed in. Now we have a spec as well that might have a default value and according @RonnyPfannschmidt and @nicoddemus 's view would override my impl one. So far the spec is used for validation only, to keep callers and impls correct signature-wise and i think we should try keep it that way. Allowing =None for spec arguments is what we minimally need to prevent calling sites to break when we add an argument. If we also move towards saying impls can only ever use None as well, then there is never a question of which default is used at an impl side.

IOW, i suggest we consider only allowing None default values for specs and maybe also for impls. Or find some other way to cater the case of wanting to have callers with different nums of arguments work.

[RonnyPfannschmidt]

@hpk42 the idea to limit the defaults only to None is pretty neat - it prevents complex missuse (that i surely would fall for at least once)

if this is documented as intentionally restricted non-neogiable way to ensure backward/forward compatibility for call sites and hook implementations it should be fine

[goodboy]

a hookimpl should be able to declare the default for backward compatibility (say a hook adds a new parameter you want to use if availiable, but still support older versions of the program that did not support/supply it

@RonnyPfannschmidt Can't we accomplish this and what @hpk42 thinks the precedence order should be by simply not strictly requiring @hookimpls to declare all kwargs declared in the hookspec?

In other words, in _MultiCall.execute() we only call the @hookimpl with keyword arguments declared on the @hookimpl that we were able to discover through pre-inspection (which we're already now doing in varnames but we just need to return defaults and assign to a new HookImpl.defaults).

From the @hookspecs perspective this means args are strictly required and kwargs are opt in. This allows for a couple interesting possibilities, namely:

• a @hookimpl can choose not to declare a kwarg it doesn't need (or hasn't yet implemented)
• if a @hookimpl defines a kwarg that is not provided by the caller, everything continues as normal and the default is used
• a hook call (_HookCaller.__call__()) can pass through new kwargs not defined in the spec which can be handled in the @hookimpls using **kwargs

As an example of a @hookimpl which utilizes all of these concepts:

 @hookimpl
 def myhookimpl(required_arg1, required_arg2, new_maybe_provided=None, **kwargs):
     # must process the required args
     do_something(arg1)
     do_something(arg2)

     if new_maybe_provided is not None:
         # new enough version that this kwarg is provided and defined in the spec
         do_something(new_maybe_provided)

      # call to hook may have passed in additional data not yet defined in the spec
      if 'my_new_kwarg' in kwargs:
          do_something(kwargs['my_new_kwarg'])
   ...

But, an older hook which declares no kwargs would also work since kwargs are opt in. So when _HookCaller.__call__() and later _MultiCall.execute() are invoked they only pass in kwargs if they are detected (through pre-inspection) as being declared on the below function.

 @hookimpl
 def myhookimpl(required_arg1, required_arg2):
     # must process the required args
     do_something(arg1)
     do_something(arg2)

# this still works since kwargs are opt in
myhookimpl(required_arg1='blah', required_arg2='doggy', new_maybe_provided=10, my_new_kwarg='yey')

The spec would look like:

@hookspec
def myhookimpl(required_arg1, required_arg2, new_maybe_provided=None):
    '''A hook that requires args 1 and 2 and a new_maybe_provided kwarg value
    '''

This ^ allows us to transition new arguments into hooks by first including them as optional kwargs (where we can then optionally emit warnings if old hooks aren't implementing them) and later transition them to required args after some known version.

... takes a breath ...

if i write an impl like def myhook(arg1, arg2=20) i kind of expect that when someone calls myhook(arg1=10) to see arg2=20 passed in.

@hpk42 yes I totally agree with you about the precedence rules. However look above ^. I do think we could make kwargs opt in for @hookimpls such that we don't pass them through unless declared in the function signature.

so far the spec is used for validation only, to keep callers and impls correct signature-wise and i think we should try keep it that way.

@hpk42 also totally agree.

Allowing =None for spec arguments is what we minimally need to prevent calling sites to break when we add an argument. If we also move towards saying impls can only ever use None as well, then there is never a question of which default is used at an impl side.

@hpk42 @RonnyPfannschmidt my only question with this approach is how we transition in new arguments gracefully and with warnings about upcoming signature changes to old hookimpls.

[RonnyPfannschmidt]

Arbitrary values make the system much harder to understand

Kwargs where abadonded because they have a massive performance toll

Your proposal grows in complexity, one of the reasons I quickly took the restriction to none is that it decouples many concerns from pluggey removing a number of mental burdens

[goodboy]

@RonnyPfannschmidt yes thinking about it more I agree. The whole point of the spec to to not allow arbitrary argument passing...
Ok so then we only allow kwargs with a default of None in both the hookimpl and the hookspec.

@RonnyPfannschmidt @hpk42 So then are you guys cool with my proposal to pre-inspect hookimpls at register time and warn for any non-declared kwargs which are declared in the hookspec?

[RonnyPfannschmidt]

@tgoodlet im not clearly understanding your differenciation wrt arguments vs kwargs

currently pluggy thinks in terms of arguments, we use names in hook invocation, but the call happens in terms of position arguments for efficiency

warnings for unknown ones ones or missing ones seems really helpfull in any case

[goodboy]

currently pluggy thinks in terms of arguments, we use names in hook invocation, but the call happens in terms of position arguments for efficiency

@RonnyPfannschmidt right but aren't we're discussing allowing hookimpls to declare keyworkd arguments (what I'm talking about when I say kwargs)?

As far as I understand this is to distinguish between newly introduced hook arguments and originally spec-ed arguments such that we can introduce new arguments to a hook spec without breaking legacy hookimpls. I can't think of any way to distinguish this difference with regular ordered args alone whilst also being able to maintain a strictly spec-ed positional arg signature. Maybe you have an idea?

Further, if we keep hook calls using positional arguments and offer up and coming new arguments through kwargs, then hookimpls declaring new kwargs must be in the correct order which may be un-intuitive given that hooks get called with keyword argument names:

• when you call a hook (hook.myhook(foo='blah', cake=10)) it doesn't seem like the hookimpl kwarg declaration order should matter but really because we call all impls with positional args it does
• if a user defines the hookimpl kwargs to be in a different order then declared in the hookspec they'll of course receive the wrong values for each

but the call happens in terms of position arguments for efficiency

@RonnyPfannschmidt I find this slightly ironic considering we literally loop through all the provided kwargs from call time before passing them as positional args to each hookimpl. I haven't benchmarked it too much but I'm pretty sure calling using a dict + the ** operator (aka func(**kwargs_from_hook_call)) is going to be faster at the C level then the func(*args_from_for_loop) + for loop we have in python which is doing essentially the exact same thing except with our own custom error on func signature mismatch.

@hpk42 I'm interested in your opinion on this performance comment ^ because I'm assuming maybe you originally had a good reason for doing it this way?

[RonnyPfannschmidt]

@tgoodlet the transformation to positional arguments was introduced to gain performance (and did work out)

i vaguely recall the issue that filtering down the dicts from all arguments to functions arguments was needed in any case (@hpk42 might recall more details)

for the sake of mental models its useful to pretend that pluggy works solely in terms of keyword arguments

[goodboy]

the transformation to positional arguments was introduced to gain performance (and did work out)

@RonnyPfannschmidt that I don't doubt. I just don't know how much faster the list comprehension + func(*list_compr_results) is then a func(**kwargs_from_call).

Here's some very rudimentary benchmarks of what's going on inside _MultiCall.execute():

In [19]: kwargs= {'doggy':10, 'kitty':'yey', 'mouse':1908}

In [20]: def f(kitty, doggy, mouse=None):
    ...:     pass
    ...: 

In [22]: fargnames = pluggy.varnames(f)

In [23]: fargnames
Out[23]: ('kitty', 'doggy')

# the current implementation
In [24]: timeit f(*[kwargs[argname] for argname in fargnames])
The slowest run took 4.67 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 556 ns per loop

In [26]: timeit f(**kwargs)
The slowest run took 6.64 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 211 ns per loop

So if we're going to keep the call by keyword argument names interface then we might gain some performance by using **.

[RonnyPfannschmidt]

@tgoodlet unfortunately you forgot to filter by applicable argument names (thats where the cost was)

[RonnyPfannschmidt]

(it might have been better to cythonize, but that would mess up vendoring in pytest)

[goodboy]

@RonnyPfannschmidt ok that's another thing. Why do we allow that - hookimpl can declare less arguments then the hookspec? Shouldn't it be strict - all hookimpl must implement all args from the spec?

Either way we still need to address how to handle kwargs declared in hookimpl for the purposes of this issue. Are you ok with the following rules:

• set(hookimpl_kwargs) < set(hookspec_kwargs) where any kwarg only found in hookspec_kwargs is warned about when hooks are verified against their spec.
• error at verify time whenever set(hookimpl_kwargs) > set(hookspec_args)

[RonnyPfannschmidt]

hooks only take arguments they need, that makes them forward compatible to the addition of new ones and removes unneeded variables from the function

for example most hooks even in pytest don't take all arguments

[goodboy]

Ahhh so args are opt in...dang.
Well I guess that's definitely something to document in #33 hehe.

@RonnyPfannschmidt in that case then I'm not totally following how you guys think this can be accomplished using kwargs assigned to None in the hookspec?

[RonnyPfannschmidt]

let me line up the cases to consider

1. hook spec grows new argument, hook caller does not add it, -> hookspec default needed
2. hook impl wants new argument if available but not break for apis where it is not supplied -> hookimpl default needed

the hook spec side is about supplying defaults in order to ensure forward compatibility for callers that pass less arguments

the hook impl side is about supplying defaults for keeping compatibility with older versions of the hookspec that did not have the argument yet

[RonnyPfannschmidt]

closing this one, i decided new hook names should be the way to go