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

Deprecate __multicall__ #58

Merged
merged 5 commits into from
Jul 16, 2017
Merged

Conversation

goodboy
Copy link
Contributor

@goodboy goodboy commented Jul 10, 2017

This starts the deprecation of __multicall__ by following the suggestions from @RonnyPfannschmidt and @hpk42 in #23:

  • if __multicall__ is detected in a hookimpl fall back to the legacy version, otherwise use the new faster implementation.

The benchmark tests show that the new _MultiCall implementation (without the recursion previously required for wrappers) outperforms the _LegacyMultiCall after more then one hook is registered (likely the short circuting of the while loop condition is faster then the multiple try/for loop fallthroughs in the new version).

For greater numbers of wrappers the new version significanly outperforms; it is also consistently faster with equivalent numbers of hook impls.

Watch the CI run for the comparisons.
Let me know what you guys think!

Tyler Goodlet added 4 commits July 7, 2017 19:26
Add a new `pluggy.callers._MultiCall` implementation which removes
support the implicit `__multicall__` special argument and drops the
recursion required to handle hook wrappers. Rename the original
implementation `_LegacyMultiCall` and load it only when the
`__multicall__` argument is detected in a hookimpl function
signature. Add a deprecation warning whenever the legacy fallback
occurs.

Resolves pytest-dev#23
@goodboy
Copy link
Contributor Author

goodboy commented Jul 11, 2017

@RonnyPfannschmidt @hpk42 I'd appreciate a look-over from you two as well :)

""" Hook was called wrongly. """


class Result(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a duplication of outcome just to remove the bad ctor
lets just move the exception handling behaviour to a alternate ctor,

so we can have _Result(result=...) or _Result.from_call(func)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I really didn't like the duplication.
@RonnyPfannschmidt are you ok with keeping _Result and being rid of _CallOutcome?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both are internal classes and the exposed api is the same, so go ahead :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RonnyPfannschmidt good enough?

@goodboy
Copy link
Contributor Author

goodboy commented Jul 11, 2017

@RonnyPfannschmidt alright changed as suggested.

Let me know if there's anything else!

`_CallOutcome` limits usage due the constructor calling a function
input. Instead add a `classmethod` constructor `_Result.from_call()`
which can be used to get the same behaviour and avoids duplicate types.
@goodboy
Copy link
Contributor Author

goodboy commented Jul 13, 2017

@RonnyPfannschmidt @hpk42 any reason I can't merge this?
I was hoping to move this forward, maybe get out another minor release, and then move towards actual removal of __multicall__ for 1.0

Let me know what you guys are thinking :)

@nicoddemus
Copy link
Member

nicoddemus commented Jul 13, 2017

I was hoping to move this forward, maybe get out another minor release, and then move towards actual removal of multicall for 1.0

Hmm how would release 1.0 affect pytest users I wonder? 🤔

@goodboy
Copy link
Contributor Author

goodboy commented Jul 13, 2017

@nicoddemus yeah no rush on that ;)

We can also fully remove it in a minor release?

@nicoddemus
Copy link
Member

yeah no rush on that ;)

Yeah, it is clear that we won't be releasing 1.0 right away, just wondering what will happen when we actually do; plugins which have been working for a long time but using __multicall__ would break.

I guess we should also start issuing DeprecationWarnings when __multicall__ is used to at least make it official.

We can also fully remove it in a minor release?

Given that this is 0.x technically we could... but this will probably force pytest to keep its vendored version pinned for a long time so we don't break things in the wild.

@goodboy
Copy link
Contributor Author

goodboy commented Jul 13, 2017

I guess we should also start issuing DeprecationWarnings when multicall is used to at least make it official.

That's already in this PR so maybe it's worth getting in the wild sooner then later?
There's no reason to rush the removal since the legacy support is of course still there.

this will probably force pytest to keep its vendored version pinned for a long time

Sure but I think getting the deprecation warning live sooner then later is probably best.
I also wonder if it's that big of a deal. __multicall__ was supposedly never officially supported and it's not like some old plugins breaking is that critical.

Maybe we could also scour github to check how many projects are still using it?

@nicoddemus
Copy link
Member

That's already in this PR so maybe it's worth getting in the wild sooner then later?

Oops my bad! I took a little quick look at master and thought we were not doing that yet.

Sure but I think getting the deprecation warning live sooner then later is probably best.

Definitely.

I also wonder if it's that big of a deal. multicall was supposedly never officially supported and it's not like some old plugins breaking is that critical.

I agree, it probably isn't that big a deal. I was just wondering aloud what would happen in 1.0 because I was under the impression you wanted that out "soonish", but I understand now that's not really the case.

In summary: definitely agree with merging this and making a new release. We should then vendor it into pytest for 3.2. 👍

@RonnyPfannschmidt
Copy link
Member

We should enable pytest to generate warnings about multicall, and drop it on 4.x as well

@nicoddemus
Copy link
Member

We should enable pytest to generate warnings about multicall, and drop it on 4.x as well

I agree with the idea, but unless I'm mistaken we don't have to actually do anything in pytest other than vendor the pluggy version which emits the warning, correct?

@RonnyPfannschmidt
Copy link
Member

@nicoddemus we need a pytest specific warning that plugin autors can rely on (and we should make plugin authors trigger the pytest ones for their own sake)

@RonnyPfannschmidt
Copy link
Member

as in the warnings in pytest need to be able to derive from PyTestWarning

@nicoddemus
Copy link
Member

Do you think that's necessary? Can't we rely on the standard deprecation warning raised by pluggy?

@RonnyPfannschmidt
Copy link
Member

if a plugin author enables pytest warnings and not pluggy warnings, he wont even see it coming and be very unhappy on 4.x

@goodboy
Copy link
Contributor Author

goodboy commented Jul 14, 2017

@RonnyPfannschmidt fwiw I used a warnings.DeprecationWarning. Are you saying some users disable this by default?

Also @RonnyPfannschmidt are you giving this a stamp of approval or no?
Again if there's anything outstanding I'm happy to make adjustments.

@RonnyPfannschmidt
Copy link
Member

@tgoodlet what i mean to say is we should enable pytest to use a subclass of PytestWarning, so pytest plugin authors can be more easily be made aware of it (its common to disable warnings you cant affect in testing so you can work on those you can affect)

@goodboy
Copy link
Contributor Author

goodboy commented Jul 14, 2017

@RonnyPfannschmidt Yeah sounds reasonable.
Is it maybe worth creating a subclass, PluggyWarning to accomplish the same?

Also not to bug but if you're happy with this PR do you mind approving?

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.

im ok with the state, i hope someone finds the time to follow up with the warnings

@goodboy
Copy link
Contributor Author

goodboy commented Jul 16, 2017

@RonnyPfannschmidt just as an FYI there already is a warning in pytest.

Not sure if that's what you're looking for or if a futher warning needs to be added?

@goodboy goodboy merged commit 6689c91 into pytest-dev:master Jul 16, 2017
@RonnyPfannschmidt
Copy link
Member

@tgoodlet thanks for bringing that up to my attention, my concerns are to be considered resolved

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

Successfully merging this pull request may close these issues.

3 participants