Skip to content

Hook wrapping without _Result#389

Merged
bluetech merged 1 commit into
pytest-dev:mainfrom
bluetech:un-result
Jun 17, 2023
Merged

Hook wrapping without _Result#389
bluetech merged 1 commit into
pytest-dev:mainfrom
bluetech:un-result

Conversation

@bluetech

@bluetech bluetech commented Jun 11, 2023

Copy link
Copy Markdown
Member

This implements the idea from #260. I think it makes pluggy more elegant, and also provides a backward compatible fix for #244 (replaces #257).

@bluetech

Copy link
Copy Markdown
Member Author

In #260, @maxnikulin argued against using a "plain" generator function for this, preferring a new @hookimpl(wrapper=True) instead, to make it explicit.

My opinion is that implicit is fine, because that's how python functions themselves work, and how e.g. pytest.fixture works, and I think they're mostly fine. But I'm willing to be outvoted on this.

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks nicer than what we have
It Will make async a little Harder, but reasonably so

@bluetech

Copy link
Copy Markdown
Member Author

It Will make async a little Harder, but reasonably so

Seems like async generators have the necessary asend/athrow, the only thing they're missing is return support. I wonder why they don't support that.

@bluetech

bluetech commented Jun 12, 2023

Copy link
Copy Markdown
Member Author

Thanks for the reviews @RonnyPfannschmidt.

I plan to:

  • Extract the force_exception change to its own PR with own changelog.
  • Try to port pytest to detect any issues and measure performance in a realistic setting.
  • Wait a week or two to allow for discussion.

@bluetech

Copy link
Copy Markdown
Member Author

Here's a branch converting all of pytest to new-style wrappers: https://github.com/bluetech/pytest/commits/new-style-wrappers

All of the tests pass, you can use it to get an impression of usage. I haven't compared performance yet.

@bluetech

Copy link
Copy Markdown
Member Author

I've made the following performance measurements:

  1. Pluggy benchmark - old pluggy/new pluggy - neutral
  2. Pytest benchmarks - old pytetst+old pluggy/new pytest+new pluggy - neutral
  3. Pytest benchmarks - old pytetst+old pluggy/old pytest+new pluggy - neutral

I also made an experiment where I removed hookwrapper=True support entirely from _multicall (removing _Result etc.) and this gets a ~15% improvement in pluggy benchmark. But we don't have to drop old-style wrappers to get this improvement; it is easy to add a fast path which specializes for only new-style case - see here (better to view ignoring whitespace). Will maybe submit this as a separate PR if this one gets merged.

@RonnyPfannschmidt

Copy link
Copy Markdown
Member

How expensive would it be to have legacy hook wrappers be implemented in terms off The new wrappers thus making them slower but allowing the new wrappers also Bugfix the exception escape

@bluetech

Copy link
Copy Markdown
Member Author

I'm not sure, but I generally think it'd be better to keep the existing hookwrapper=True semantics as-is, to avoid breaking things (https://xkcd.com/1172/ situation). Switching to the new style would also be opting in to the new semantics.

@nicoddemus

nicoddemus commented Jun 13, 2023

Copy link
Copy Markdown
Member

Great work @bluetech!

The idea seems great, I will take a look at the code in the next few days! 👍

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great, thanks @bluetech!

Comment thread docs/index.rst
"""Wrap calls to ``setup_project()`` implementations which
should return json encoded config options.
"""
# get initial default config

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For consistency, the legacy example in Old-style wrappers could also have the exact same beginning as here: defaults = config.tojson(), and then printing the debug info via if config.debug.

(I would suggest that directly below, but I cannot leave a suggestion outside the diff)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops I forgot to handle this -- fixing now.

Fix pytest-dev#260.

Co-authored-by: Maxim Nikulin <manikulin@gmail.com>
@bluetech

Copy link
Copy Markdown
Member Author

Thanks for the reviews!

After taking another look at #257, I've made a few changes:

  • Added the teardown.close() bit -- it's good to have. Added it only for the new style so as not to touch old style.
  • Ported the tests from there.
  • Added @maxnikulin as co-author

@bluetech bluetech merged commit 40fbab1 into pytest-dev:main Jun 17, 2023
@bluetech bluetech deleted the un-result branch June 17, 2023 14:44
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