Drop multicall by goodboy · Pull Request #147 · pytest-dev/pluggy

goodboy · 2018-05-18T01:33:26Z

Just to get some eyes on it as we approach 1.0!

nicoddemus

LGTM!

Related to #59 btw.

RonnyPfannschmidt · 2018-05-18T04:49:04Z

it should be noted that we require a breaking pytest release to update to this one

nicoddemus · 2018-05-18T12:24:22Z

it should be noted that we require a breaking pytest release to update to this one

On pytest we probably will no longer pin pluggy after 1.0, so not sure we need to change pytest at all... users will be able to pin to pluggy<1.0 if they require to.

obestwalter · 2018-05-18T13:52:44Z

Same for tox, I guess :)

goodboy · 2018-05-18T14:06:38Z

@RonnyPfannschmidt yes @nicoddemus makes a good point: pytest need not be concerned with whether users still require __multicall__ since plugins can simply pin to pluggy <= 1.0 and everything should just work. That's the nice part of the graceful fade out of this subsystem.

RonnyPfannschmidt · 2018-05-18T19:33:15Z

@tgoodlet @nicoddemus that ignores pytest possibly wanting to use new features of pluggy and the fact that pluggy is part of the public api of pytest, as such, break a part break the whole

i'd like to be pedantic about things there

bluetech · 2020-06-02T18:31:59Z

Since pytest 6.0 is coming, maybe it's a good time to merge this, release pluggy 1.0 and require pluggy>=1.0,<2.0 in pytest 6.0? (I can do the pytest part)

goodboy · 2020-06-02T18:34:42Z

@bluetech I'm in.

This has been getting put off for no real reason really.
I'll do the rebase onto recent master once back at the keyboard.

Drop the `_LegacyMultiCall` type and it's recursion "fun". We've got a faster and simpler function-loop approach now with `pluggy.callers._multicall()` it's been doing great in production! Resolves pytest-dev#59

goodboy · 2020-06-03T16:25:42Z

Just working out some kinks from the rebase onto master.

goodboy · 2020-06-03T17:12:22Z

Ok think I got it all.

One question, should I start a 1.0 tag here to get a dev version going?

asottile

bluetech

Left some suggestions, other than that, LGTM!

bluetech · 2020-06-03T18:07:36Z

    return [wrapper for i in range(request.param)]


-@pytest.fixture(params=[_multicall, _legacymulticall], ids=lambda item: item.__name__)


I figure this fixture isn't really needed any more.

Lel, good point.

bluetech · 2020-06-03T18:08:28Z

        hookfuncs.append(f)
-        if "__multicall__" in f.argnames:
-            caller = _legacymulticall
    return caller(hookfuncs, kwargs, firstresult=firstresult)


No need for the caller indirection anymore.

Woo yeah this should be a nice cleanup 😸

bluetech · 2020-06-03T18:09:32Z

-                "removed in an upcoming release.",
-                DeprecationWarning,
-            )
-            self.multicall = _legacymulticall


Probably the self.multicall indirection is not needed anymore -- can call _multicall directly?

@bluetech actually, come to think of this I wonder if it makes sense to keep this in preparation for something like #50? The overloading of the multi-caller I guess can be done anywhere?

I'd like @RonnyPfannschmidt's opinion on this.

Also this might bring up discussion on whether the PluginManager._inner_hookexec is something we could also get rid of since it only seems to be used for tracing iirc. Might be better addressed in a new issue/discussion tho.

actually, come to think of this I wonder if it makes sense to keep this in preparation for something like #50?

I'd say remove it and let #50 add it back, to show the "real cost", but up to you.

Also this might bring up discussion on whether the PluginManager._inner_hookexec is something we could also get rid of since it only seems to be used for tracing iirc

pytest seems to use the tracing stuff (add_hookcall_monitoring)

pytest seems to use the tracing stuff

Definitely, I've just wondered about implementing the tracing as part of the hook caller instead of some wrapper slapped in around the _hookexec() by the PluginManager.

Looking at this again I'm more convinced all that tracing magic should be implemented on the _HookCaller including adding a method to do the .subset_hook_caller() stuff. In particular it's currently limiting us from doing per-hook tracing as well.

If you want to do tracing around the caller I'm not sure why that has anything to do (abstraction wise) with the plugin manager - the only relation really is the global context of tracing all hooks that is currently supported.

@bluetech either way I dropped the _HookCaller.multicall here.

Made #262 regarding my 2nd last comment.

bluetech · 2020-06-03T18:11:50Z

@@ -0,0 +1,2 @@
+Remove internal usage of legacy ``__multicall__`` recursive hook calling system.


Unless I'm misunderstanding, this is a public API, not just internal usage, so

Suggested change

Remove internal usage of legacy ``__multicall__`` recursive hook calling system.

Remove legacy ``__multicall__`` recursive hook calling system.

@bluetech actually I'm pretty sure this was always an internal system used as a hack, which is why we were also able to remove it.

I think @RonnyPfannschmidt might be able to comment since he has seniority 😼

bluetech · 2020-06-03T18:15:22Z

One question, should I start a 1.0 tag here to get a dev version going?

I think it would be good to have pre-release versions, we can change pytest master to use them.

goodboy · 2020-06-03T18:40:20Z

One question, should I start a 1.0 tag here to get a dev version going?

I think it would be good to have pre-release versions, we can change pytest master to use them.

Indeed @bluetech. Is a beta0 tag good enough: 1.0.beta0 as per PEP 440 or something similar maybe?

bluetech · 2020-06-03T18:47:13Z

Is a beta0 tag good enough: 1.0.beta0 as per PEP 440 or something similar maybe?

From the link you provided, looks like it should rather be 1.0.0b1.

I think alpha or beta is appropriate (compared to the "Developmental releases" which are described as regularly occurring things).

goodboy · 2020-06-03T18:49:25Z

@bluetech fwiw I'm pretty sure setuptools will transform 1.0.0beta0 -> 1.0.0b0.

compared to the "Developmental releases" which are described as regularly occurring things).

Plus I think setuptools_scm already uses the dev tags for rolling development releases based on the git hash.

RonnyPfannschmidt · 2020-06-03T20:23:06Z

Setuptools_scm supports a .dev tag suffix to denote a new development epoch

goodboy · 2020-06-03T21:34:10Z

Just waiting on what tag I should push team 😄

RonnyPfannschmidt · 2020-06-04T05:58:30Z

i would propose 1.0.dev to be inserted on master after merge

nicoddemus approved these changes May 18, 2018

View reviewed changes

goodboy force-pushed the drop_multicall branch from 3c22562 to 66a0590 Compare May 22, 2018 18:30

goodboy added this to the 1.0 milestone Oct 16, 2018

bluetech mentioned this pull request Aug 16, 2019

Avoid cyclic dependency by merging callers.py into hooks.py #227

Closed

Tyler Goodlet added 2 commits June 3, 2020 12:01

Drop __multicall__!

b267687

Drop the `_LegacyMultiCall` type and it's recursion "fun". We've got a faster and simpler function-loop approach now with `pluggy.callers._multicall()` it's been doing great in production! Resolves pytest-dev#59

Drop __multicall__ from tests

d59403b

goodboy force-pushed the drop_multicall branch from 66a0590 to d59403b Compare June 3, 2020 16:11

Tidying to appease black

accb6d6

goodboy added 2 commits June 3, 2020 13:09

Drop argname from spec type (it was added later)

62dae61

Remove deprecation warning test

74cde77

goodboy requested review from RonnyPfannschmidt, bluetech and nicoddemus June 3, 2020 17:11

Drop unneeded f-strings

95bbf5e

goodboy force-pushed the drop_multicall branch from 1013ebe to 95bbf5e Compare June 3, 2020 17:24

goodboy requested a review from asottile June 3, 2020 17:41

goodboy mentioned this pull request Jun 3, 2020

Handle exceptions in hookwrapper post stage (#244) #257

Closed

asottile approved these changes Jun 3, 2020

View reviewed changes

bluetech reviewed Jun 3, 2020

View reviewed changes

bluetech mentioned this pull request Jun 3, 2020

testing: change a test to not use deprecated pluggy __multicall__ protocol pytest-dev/pytest#7315

Merged

goodboy added 2 commits June 3, 2020 16:18

Add news fragment

b39f373

Drop callertype fixture from benchmark tests

72948af

goodboy force-pushed the drop_multicall branch from 7fe63d8 to 72948af Compare June 3, 2020 20:18

Drop _HookCaller.multicall indirection

bcbcdc0

goodboy force-pushed the drop_multicall branch from e8f93d0 to bcbcdc0 Compare June 3, 2020 21:14

RonnyPfannschmidt approved these changes Jun 4, 2020

View reviewed changes

bluetech approved these changes Jun 4, 2020

View reviewed changes

goodboy merged commit 016e410 into pytest-dev:master Jun 4, 2020

This was referenced Jun 4, 2020

Cythonize the call loop #263

Closed

Release 1.0? #140

Closed

		return [wrapper for i in range(request.param)]


		@pytest.fixture(params=[_multicall, _legacymulticall], ids=lambda item: item.__name__)

		@@ -0,0 +1,2 @@
		Remove internal usage of legacy ``__multicall__`` recursive hook calling system.

	Remove internal usage of legacy ``__multicall__`` recursive hook calling system.
	Remove legacy ``__multicall__`` recursive hook calling system.

Conversation

goodboy commented May 18, 2018

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt commented May 18, 2018

Uh oh!

nicoddemus commented May 18, 2018

Uh oh!

obestwalter commented May 18, 2018

Uh oh!

goodboy commented May 18, 2018

Uh oh!

RonnyPfannschmidt commented May 18, 2018

Uh oh!

bluetech commented Jun 2, 2020

Uh oh!

goodboy commented Jun 2, 2020

Uh oh!

goodboy commented Jun 3, 2020

Uh oh!

goodboy commented Jun 3, 2020

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodboy Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluetech commented Jun 3, 2020

Uh oh!

goodboy commented Jun 3, 2020

Uh oh!

bluetech commented Jun 3, 2020

Uh oh!

goodboy commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonnyPfannschmidt commented Jun 3, 2020

Uh oh!

goodboy commented Jun 3, 2020

Uh oh!

RonnyPfannschmidt commented Jun 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

goodboy Jun 3, 2020 •

edited

Loading

goodboy commented Jun 3, 2020 •

edited

Loading