support defaults to enable deprecation about pluggy HOT 48 CLOSED

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.

from pluggy.

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

atm its missing a actually sane way to transfer the reason

from pluggy.

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.

from pluggy.

@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

from pluggy.

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.

from pluggy.

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

from pluggy.

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.

from pluggy.

@tgoodlet this one might be a bit tricky

from pluggy.

@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

from pluggy.

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.

from pluggy.

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.

from pluggy.

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.

from pluggy.

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 ^^

from pluggy.

@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.

from pluggy.

@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

from pluggy.

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.

from pluggy.

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

from pluggy.

@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?

from pluggy.

@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

from pluggy.

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?

from pluggy.

@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

from pluggy.

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 **.

from pluggy.

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

from pluggy.

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

from pluggy.

@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)

from pluggy.

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

from pluggy.

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?

from pluggy.

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

from pluggy.

@RonnyPfannschmidt OK so with #34 I've added at the least warnings for mismatching call - hookspec situations. But I don't know that this has gotten any closer to what you actually need in terms of defaults.

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

There seems to be 2**2 = 4 major possibilities:

1. call matches hookspec
2. call doesn't match hookspec (2 reasons)
   1. hook call is older then hookspec and doesn't include new args
   2. hook call is newer then hookspec and includes new args not in some older spec
3. hookimpl matches hookspec
4. hookimpl doesn't match hookspec (2 reasons)
   1. hookspec is older version then latest
   2. hookimpl doesn't use all args in the spec

I'm trying to understand how you and @hpk42 think this can be accomplished with =None kwargs in hookspecs. Afaik right now hookimpl can already declare kwargs without problem.

from pluggy.

@tgoodlet the case for hookspec is the following: 3rd party plugin calls hook with old arguments, the new ones are missing

from pluggy.

@tgoodlet not sure if you have read but there is an extensive module docstring in pluggy.py which i recommend to study.

So i think the main new thing this issue is about is allowing a hook call where the caller provides less arguments than the spec. For this we need to mark hookspec arguments as optional. Hook implementations then need to be able to accept such an optional argument and detect if it was provided from the caller or not. At the same time a hook implementation might want to work with both the old and the new hookspec and thus only optionally accept the optional argument.

Is this what we are still talking about in this issue?

from pluggy.

from my pov this one derailed, i will create a pull request with xfailing tests for the cases i talked about

from pluggy.

@hpk42 I thought I had read it but, maybe I better give another look.

So i think the main new thing this issue is about is allowing a hook call where the caller provides less arguments than the spec. For this we need to mark hookspec arguments as optional.

@hpk42 Ok now I'm confused.
As far as I can tell this is already allowed? When _HookCaller.__call__() is invoked there's never any checking against the spec. This is why I added warnings in #34.

So right now all hookspec args are optional. That is, a hookimpl only must declare a subset of them. The only case you'll get an error is when an impl has more arguments then what's provided by the caller.

Hook implementations then need to be able to accept such an optional argument and detect if it was provided from the caller or not. At the same time a hook implementation might want to work with both the old and the new hookspec and thus only optionally accept the optional argument.

@hpk42 yes this is the tricky part and would be the reason to use a kwarg in the hookimpl. But what I've been trying to say is that hookimpl could always do this.
You're perfectly allowed to define a hookimpl like the following:

@hookimpl
def myhook(doggy, kitty, reason="The default reason"):
    ...

All that needs to change is the logic in _MultiCall.execute():

• any kwarg values defined in the hookimpl (in this case ^ reason) should receive values from the caller if provided (so we don't error like in the args case)

^ turns out to be very awkward though because of the way we call the end functions with *args...

res = hook_impl.function(*args)

I think @RonnyPfannschmidt is right this can only be further discussed with code.

from pluggy.

@tgoodlet we seem to be using different language. There are three types of things:

• a hook spec
• a hook call
• hook implementations

So when i say "a hook caller providing less arguments then the spec" i am not talking about any implementation. In fact there could be zero hook impls and pluggy would still throw a HookCallError. I am talking about e.g. myhook() throwing an error because the spec said myhook(arg1) i.e. requires an argument. A hook call and a spec must currently match. See e.g. https://github.com/pytest-dev/pluggy/blob/master/testing/test_pluggy.py#L288

from pluggy.

@hpk42 That test you linked to only throws an error because the hookimpl declares the same args as the spec and the hook caller specifies less args then the impl. If the impl didn't declare any any arguments the call would still work without error. In fact from call time onward there is never any checking against the hookspec.

Anyway I think I got in what @RonnyPfannschmidt originally had in mind in #34 although I still don't think the implementation is ideal nor are the hook call semantics clear.

from pluggy.

@tgoodlet good catch -- i do think that calls should be matched against the spec. I see it as a bug that this is not the case.

from pluggy.

@hpk42 agreed and that's what I've done in #34 where we now throw a warning if the hookspec and hook call signatures don't match. I don't think we can throw an error if, as @RonnyPfannschmidt mentioned, we intend to support old hook calls which may not always match the latest hookspec.

from pluggy.

@hpk42 @RonnyPfannschmidt hehe so the kwarg=None declared in the hookimpl approach won't work either right now because we discard results that return None currently...

from pluggy.

@RonnyPfannschmidt @hpk42 ping? Any news on this?

from pluggy.

nope, havent had time to work on it further

from pluggy.

@RonnyPfannschmidt you saw that I implemented your original requirements in #34 though right?

from pluggy.

@tgoodlet i indeed missed that, first glance impression looks good, ill take a closer look later (i believe 1 test is missing)

from pluggy.

@RonnyPfannschmidt alright glad I asked then :)

I also mentioned in #34 that I think we might want to use the func(**kwargs) approach instead as it will simplify the code and I don't it think actually causes that much of a performance hit (hence #37).

from pluggy.

Let's solve those concerns distinct, I'm fairly certain kwargs do have a massive impact

from pluggy.

I might not get to it until after Christmas

from pluggy.

@RonnyPfannschmidt @hpk42 @nicoddemus should I maybe break up my PR #34 for the separate issues?

That is I'm solving both:

• warning when a hook call does not match the hookspec (implemented with this commit)
• providing defaults from a hookspec's kwargs (implemented in this one)

I'm thinking having too much code in this review is making this already confusing task harder to grok.

from pluggy.

Hey @tgoodlet, sorry about not being able to review your PR yet, trying to caught up with some other pending issues/reviews/implementations in other projects.

But if you want to split the PR (and it is not too much work for you to do), I think it would indeed simplify reviewing IMO.

from pluggy.

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

from pluggy.