Improved fixture reuse by new param keys that can be derived from API ids #9420

haxtibal · 2021-12-16T21:43:59Z

This is a less trivial but (arguably) more powerful alternative to #9350. It fixes #8914. For a review, please follow the commit series and messages.

Situation: Both reorder_items and FixtureDef.execute must decide and agree about whether fixture instances can be reused. The obvious is to decide by comparing parameter values. Sadly none of prm1 == prm2, prm1 is prm2 or hash(prm1) == hash(prm2) is generally suited to decide about "are parameters the same?":

dicts are not hashable
numpy arrays don't compare to bool, and are not hashable either
objects compare and hash by identity, but a user may expect reuse only if it's equal by value

Using the parameter index instead of value doesn't work either, because, well, see #8914.

Idea: Give users the option to explicitly control equality of parameters by leveraging parameter ids. Under the hood we introduce a param_key to support fixture reordering and caching. The key is built according to the following rules

If parameter ids have been passed via API, use that parameter id as key.
Else, if a parameter value is hashable, use that parameter value as key.
Else, fallback to a parameter values identity.

Example 1: Use id= to control equality of the unhashable dict parameters (rule 1 applies).

@pytest.fixture(scope="session")
def foo(request.param):
    pass  # goal: minimize nrof setup/teardown
@pytest.mark.parametrize("foo", [pytest.param({"data": "a"}, id="0"), pytest.param({"data": "b"}, id="1")], indirect=True)
def test1(foo):
    pass
@pytest.mark.parametrize("foo", [pytest.param({"data": "b"}, id="1"), pytest.param({"data": "a"}, id="0")], indirect=True)
def test2(foo):
    pass

Example 2: No need to use id= or ids= if the parameter is hashable by value (rule 2 applies).

@pytest.fixture(scope="session")
def foo(request.param):
    pass  # goal: minimize nrof setup/teardown
@pytest.mark.parametrize("foo", ["a", "b"], indirect=True)
def test1(foo):
    pass
@pytest.mark.parametrize("foo", ["b", "a"], indirect=True)
def test2(foo):
    pass

Also related: #244, #5693, #6497, #6541.

src/_pytest/python.py

testing/python/fixtures.py

src/_pytest/python.py

bluetech · 2021-12-23T08:56:58Z

@haxtibal Thanks a lot for creating this separate PR with nice commits and all!

I'm looking forward to reviewing this PR, when I have the time available. Right now we are mostly focused on getting the 7.0 release out the door, just wanted to write this so you don't get discouraged :)

src/_pytest/fixtures.py

bluetech

I think I'm going to leave a separate review for each commit. This is for "Refactor idmaker functions into class IdMaker'.

The refactoring LGTM, with some comments.

bluetech · 2022-01-21T13:31:13Z

src/_pytest/python.py

@@ -911,6 +911,124 @@ def hasnew(obj: object) -> bool:
    return False


+@final
+@attr.s(frozen=True, auto_attribs=True)


Can add slots=True here as well, doesn't hurt.

doesn't hurt

If I understand attrs docs correctly, @attr.s(frozen=True, slots=True, ...) actually could hurt in some cases:

You should avoid instantiating lots of frozen slotted classes (i.e. @Frozen) in performance-critical code

I can't reason offhand about whether we'd be affected, can you?

Hmm interesting. We use this all over the place so we should probably check if it has any effect separately. But until then let's keep it in this PR.

src/_pytest/python.py

bluetech

Some more comments on the first commit. Sorry for all of this, I just hate to pass on an opportunity to make some pytest code clearer :)

If it's too much I can cherry pick the commit to a separate PR along with my suggestions.

src/_pytest/python.py

bluetech

Review for "Add IdMaker.make_parameter_keys".

The commit message note writes pytest.param("apple", ids=["fruit"]) but meant to write pytest.param("apple", id="fruit").

Regarding the the problem with the duplicates, when there are conflicts in make_unique_test_ids pytest disambiguates them with a suffix. This should probably also be done for parameter keys then, since duplicates probably don't make sense within the same group of parameter sets.

src/_pytest/python.py

bluetech · 2022-01-21T15:10:31Z

src/_pytest/python.py

+    def make_parameter_keys(self) -> Iterable[Dict[str, Hashable]]:
+        """Make hashable keys for parameters in a parametrized test.
+
+        This key will be considered to determine if parameters


This key will be considered (along with the parameter name) to determine ...

(along with the parameter name)

What is the "parameter name"? reorder_items uses e.g. key = (argname, param_key, item.path, item_cls), and FixtureDef.cache_key uses param_key. Do you mean argument name?

src/_pytest/python.py

bluetech

Review of "Extend CallSpec2 with param_keys".

LGTM, though note #9531 causes some conflicts, sorry about that.

bluetech

"Extend SubRequest with param_key" LGTM.

bluetech

"Let reorder_items use our new parameter key" LGTM with comment

src/_pytest/fixtures.py

bluetech

Review for " Let FixtureDef.cache_key use our new parameter key "

src/_pytest/fixtures.py

bluetech · 2022-01-21T15:56:56Z

I reviewed everything, though I ran out of time toward the end. The commit separation and commit messages were very helpful.

haxtibal · 2022-01-22T20:05:45Z

Regarding the the problem with the duplicates, when there are conflicts in make_unique_test_ids pytest disambiguates them with a suffix. This should probably also be done for parameter keys then, since duplicates probably don't make sense within the same group of parameter sets.

For make_parameter_keys, leaving duplicates duplicated is at the heart of what makes the new feature work: Only duplicates hash to the same value, and same hash will in later stages be the criterion to skip all but one duplicate for the sake of fixture s/t optimization. Does this reasoning make sense?

haxtibal · 2022-01-23T21:06:12Z

@bluetech Thanks for taking your time! That was a really helpful review.

Most of the remarks are hopefully done, and the commit series is rebased on latest main. I left conversations open where response from you would be good.

bluetech

Second review of the " Refactor idmaker functions into class IdMaker " commit.

I left a few comments, but it looks good to me now. It's nice regardless, so if you agree, I can cherry-pick it to main already, to reduce the size of the remaining PR.

I will try to review the rest again soon.

src/_pytest/python.py

haxtibal · 2022-01-26T09:23:47Z

if you agree, I can cherry-pick it to main already, to reduce the size of the remaining PR.

Just applied your recent comments. Sure, cherry-pick it, or in case you find some more bits you'd like to change, it's fine for me if you amend the commit to spare another review iteration on "Refactor idmaker functions into class IdMaker".

These parameter keys will later become the unified way how reorder_items and FixtureDefs decide if parameters are equal and can be reused. Order for what to use as key is as follows: 1. If users gave explicitly parameter ids, use them as key. 2. If not explictely given, and the parameter value is hashable, use the parameter value as key. 3. Else, fallback to the parameters identity. NB: Rule 1 gives users ultimate (equallity-telling) power, and with great power comes great responsiblity. One could now do something wired like @pytest.mark.parametrize(fruit, [ pytest.param("apple", id="fruit"), pytest.param("orange", id="fruit"), ] def test_fruits(fruit): pass The user just made "apple" equal to "orange". If that's what they intend is unknown, but probably not.

This way we make the previously calculated parameter key accessible to reorder_items and FixtureDef.

Pick up the value from curent CallSpec2 and assign it to the SubRequest. It's required to make the parameter key accessible in FixtureDef.execute.

Fixes test reordering for indirect parameterization (see pytest-dev#8913). Prior to this commit, reorder_items considered the parameter index to tell if a parameter is "the same" and therefore can be shared. Looking at the index causes trouble if there are multiple parametrizations for the same fixture, basically because one index means different things in different parameter lists. This is fixed here by using the recently introduced parameter key as grouping criterion. Caution: The parameter key ends up inside the key of another dict, and therefore must be hashable. CallSpec2.param_keys is crafted sufficiently, it guarantees to contain comparable and hashable values.

The FixtureDef cache must agree with reorder_items about what parmeters are the same. The new param key must (and can) be compared by value, so we change from "is" to "==" in FixtureDef.execute.

Add tests to assert pytest-dev#8914 is fixed. Tests assures that both reordering and caching work as intended, and demonstrates how one can use parameter ids to decide about equality. Adapting issue_519.checked_order is not cheating. See our discussion at pytest-dev#9350 (review)

bluetech · 2022-01-27T10:55:06Z

(I rebased on top of the cherry-picked commit)

bluetech · 2022-02-12T20:53:51Z

Gave this another (quick) look.

First, the example in the first commit message is not very fitting, because @pytest.mark.parametrize() parametrizations are function scoped, so they don't get a chance to be reused, i.e. not relevant for this change. I suggest the following example instead (prints "apple" twice):

import pytest

@pytest.fixture(params=["apple", "orange"], ids=["fruit", "fruit"], scope="module")
def a_fruit(request):
    return request.param

def test_fruits(a_fruit):
    print(a_fruit)

This makes me think, if we shouldn't add a "deduplication" step to make_parameter_keys, similar to the one in make_unique_parameterset_ids, to prevent such cases? Because I mean, it does not make much sense for someone to duplicate an id within a single parametrization (basically, for a single test)? The mechanism here is only useful for better managing the lifetime of higher-scoped indirect fixtures across tests.

And that made me further think, if we shouldn't just completely do away with falling back on comparing values (with the entire SafeHashWrapper business it entails), and instead always generate string IDs (basically, call _idval in _parameter_keys_from_parameterset). This will mostly keep the existing behavior (since _idval_from_argname eventually falls back to the parameterset idx), but still allow the user to override with an explicit ID (which we could recommend they do).

This stuff is pretty entangled and complicated so I may be confused or missing something, but WDYT?

haxtibal · 2022-02-19T14:38:47Z

@bluetech Sorry for delay in response.

the example in the first commit message is not very fitting

Agreed, my example not only missed scope="module", but also contained syntax errors.

I suggest the following example [...]

Agreed. That's better suited.

it does not make much sense for someone to duplicate an id within a single parametrization (basically, for a single test)?

Agreed. I'd suggest to print a warning or error message then, and maybe even bail out early, instead of deduplicating silently?

The mechanism here is only useful for better managing the lifetime of higher-scoped indirect fixtures across tests.

Agreed. Right now I'm slightly puzzled why direct parametrization (as in my commit example) is subject to fixture lookup at all - as opposed to unconditionally take values from CallSpec2.funcargs. Because, as you said, there's just no way to optimize something for direct parametrization. It's not new behavior from this PR (I hope), I just need to think twice to maybe understand the original intent.

And that made me further think, if we shouldn't just completely do away with falling back on comparing values (with the entire SafeHashWrapper business it entails), and instead always generate string IDs (basically, call _idval in _parameter_keys_from_parameterset). This will mostly keep the existing behavior (since _idval_from_argname eventually falls back to the parameterset idx), but still allow the user to override with an explicit ID (which we could recommend they do).

This stuff is pretty entangled and complicated so I may be confused or missing something, but WDYT?

We can't simply rely on _idval, as the string it generates is not guaranteed to be unique within fixture scope. If we do what you suggested we can get incorrect parametrization:

import pytest

@pytest.fixture(scope="module")
def fruitfix(request):
    print(f"setup fruitfix with {request.param}")
    return request.param

@pytest.mark.parametrize("fruitfix", [(1, 2), (3, 4)], indirect=True)
def test_fruits_1(fruitfix):
    print(f"test1 using {fruitfix}")

@pytest.mark.parametrize("fruitfix", [(3, 4), (5, 6)], indirect=True)
def test_fruits_2(fruitfix):
    print(f"test2 using {fruitfix}")

Output with _evaluate_idval_function, or SafeHashWrapper if the former is None (the PRs current approach):

setup fruitfix with (1, 2)
test1 using (1, 2)
setup fruitfix with (3, 4)
test1 using (3, 4)
test2 using (3, 4)
setup fruitfix with (5, 6)
test2 using (5, 6)

Output with using _idval as suggested (note how tuple (5, 6) is not passed to test2):

setup fruitfix with (1, 2)
test1 using (1, 2)
test2 using (1, 2)
setup fruitfix with (3, 4)
test1 using (3, 4)
test2 using (3, 4)

This is because _idval ~~only generates unique value representations for scalar parameters like int, float, double. But not for e.g. tuples, where it would~~ falls back to argument names + counter for structured types. Further, it would stringify scalar types str("1.23") and float(1.23) to the same id "1.23". Therefore _idval suffers from a similar problem as using the index (the strategy before this PR).

What we could do was to use _evaluate_idval_function instead of _idval, and fallback to id() right away instead of SafeHashWrapper. This would run correctly, but we lose fixture optimization ~~for tuple types~~. The advice to the user would then be simply: "Provide ids, else you won't get optimized".

haxtibal · 2022-02-20T10:12:56Z

but WDYT?

Sorry for the edits in yesterdays answer. They make decision even simpler: We can trade the current SafeHashWrapper approach for a documentation statement "Provide ids, else your parametrized fixtures won't get optimized".

My personal opinion is to keep SafeHashWrapper and the implied compare-by-value. When I came to pytest I was under the impression that reuse of parametrized fixtures is an advertised feature and found it to be the most outstanding one compared to other testing frameworks (although it mostly didn't work because of a bug). As a user, I would like if it works without custom annotations where possible.

But I can understand if you want to leave the compare-by-value fallback out: It's simpler to document and understand, and it's probably simpler to maintain.

@bluetech It's your turn to decide, I will for sure be fine with either decision.

nicoddemus · 2022-05-31T19:38:05Z

@bluetech @haxtibal gentle ping to get this rolling again (or close, if deemed best).

haxtibal · 2022-05-31T20:36:19Z

@bluetech @haxtibal gentle ping to get this rolling again (or close, if deemed best).

I'd still be interested and standing by. @bluetech How about you?

obestwalter · 2024-06-20T10:00:29Z

@bluetech would you want to have another look at this. Or otherwise this might better be closed?

haxtibal mentioned this pull request Dec 16, 2021

Fix test reordering for indirect parameterization #9350

Closed

haxtibal commented Dec 18, 2021

View reviewed changes

src/_pytest/python.py Outdated Show resolved Hide resolved

haxtibal commented Dec 18, 2021

View reviewed changes

testing/python/fixtures.py Outdated Show resolved Hide resolved

haxtibal force-pushed the feature/parameter_keys branch from 98ce476 to 099e4d1 Compare December 18, 2021 20:52

haxtibal commented Dec 19, 2021

View reviewed changes

src/_pytest/python.py Outdated Show resolved Hide resolved

haxtibal commented Dec 23, 2021

View reviewed changes

src/_pytest/fixtures.py Outdated Show resolved Hide resolved

haxtibal commented Dec 23, 2021

View reviewed changes

src/_pytest/fixtures.py Outdated Show resolved Hide resolved

bluetech reviewed Jan 21, 2022

View reviewed changes

src/_pytest/python.py Outdated Show resolved Hide resolved

src/_pytest/python.py Outdated Show resolved Hide resolved

src/_pytest/python.py Outdated Show resolved Hide resolved

src/_pytest/python.py Outdated Show resolved Hide resolved

bluetech reviewed Jan 21, 2022

View reviewed changes

src/_pytest/fixtures.py Outdated Show resolved Hide resolved

bluetech reviewed Jan 21, 2022

View reviewed changes

src/_pytest/fixtures.py Outdated Show resolved Hide resolved

src/_pytest/fixtures.py Show resolved Hide resolved

haxtibal force-pushed the feature/parameter_keys branch 4 times, most recently from ff42377 to a700b05 Compare January 23, 2022 20:49

bluetech mentioned this pull request Jan 25, 2022

fixtures: document FixtureDef's attributes #9546

Merged

bluetech reviewed Jan 25, 2022

View reviewed changes

src/_pytest/python.py Outdated Show resolved Hide resolved

src/_pytest/python.py Outdated Show resolved Hide resolved

src/_pytest/python.py Outdated Show resolved Hide resolved

src/_pytest/python.py Outdated Show resolved Hide resolved

src/_pytest/python.py Outdated Show resolved Hide resolved

haxtibal force-pushed the feature/parameter_keys branch from a700b05 to 3274429 Compare January 26, 2022 09:15

bluetech mentioned this pull request Jan 26, 2022

Refactor idmaker functions into class IdMaker #9547

Merged

Tobias Deiminger added 3 commits January 27, 2022 11:37

Extend CallSpec2 with param_keys

a8151fb

This way we make the previously calculated parameter key accessible to reorder_items and FixtureDef.

Extend SubRequest with param_key

94279c0

Pick up the value from curent CallSpec2 and assign it to the SubRequest. It's required to make the parameter key accessible in FixtureDef.execute.

Tobias Deiminger added 4 commits January 27, 2022 11:37

Let FixtureDef.cache_key use our new parameter key

5377ff1

The FixtureDef cache must agree with reorder_items about what parmeters are the same. The new param key must (and can) be compared by value, so we change from "is" to "==" in FixtureDef.execute.

Update changelog and AUTHORS

26ae398

bluetech force-pushed the feature/parameter_keys branch from 3274429 to 26ae398 Compare January 27, 2022 10:52

bluetech mentioned this pull request Jul 15, 2023

Make high-scope fixtures teardown at last dependent test #10771

Closed

bluetech mentioned this pull request Sep 9, 2023

Improvement: Base FixtureArgKeys on param values if possible, not param indices #11271

Open

obestwalter added the status: needs information reporter needs to provide more information; can be closed after 2 or more weeks of inactivity label Jun 20, 2024

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jun 20, 2024

bluetech mentioned this pull request Jul 16, 2024

fix: improved caching of parameterized fixtures #12600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved fixture reuse by new param keys that can be derived from API ids #9420

Improved fixture reuse by new param keys that can be derived from API ids #9420

haxtibal commented Dec 16, 2021

bluetech commented Dec 23, 2021

bluetech left a comment

bluetech Jan 21, 2022

haxtibal Jan 22, 2022 •

edited

Loading

bluetech Jan 25, 2022

bluetech left a comment

bluetech left a comment

bluetech Jan 21, 2022

haxtibal Jan 22, 2022

bluetech left a comment

bluetech left a comment

bluetech left a comment

bluetech left a comment

bluetech commented Jan 21, 2022

haxtibal commented Jan 22, 2022

haxtibal commented Jan 23, 2022

bluetech left a comment

haxtibal commented Jan 26, 2022

bluetech commented Jan 27, 2022

bluetech commented Feb 12, 2022

haxtibal commented Feb 19, 2022 •

edited

Loading

haxtibal commented Feb 20, 2022

nicoddemus commented May 31, 2022

haxtibal commented May 31, 2022

obestwalter commented Jun 20, 2024 •

edited

Loading

Improved fixture reuse by new param keys that can be derived from API ids #9420

Are you sure you want to change the base?

Improved fixture reuse by new param keys that can be derived from API ids #9420

Conversation

haxtibal commented Dec 16, 2021

bluetech commented Dec 23, 2021

bluetech left a comment

Choose a reason for hiding this comment

bluetech Jan 21, 2022

Choose a reason for hiding this comment

haxtibal Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

bluetech Jan 25, 2022

Choose a reason for hiding this comment

bluetech left a comment

Choose a reason for hiding this comment

bluetech left a comment

Choose a reason for hiding this comment

bluetech Jan 21, 2022

Choose a reason for hiding this comment

haxtibal Jan 22, 2022

Choose a reason for hiding this comment

bluetech left a comment

Choose a reason for hiding this comment

bluetech left a comment

Choose a reason for hiding this comment

bluetech left a comment

Choose a reason for hiding this comment

bluetech left a comment

Choose a reason for hiding this comment

bluetech commented Jan 21, 2022

haxtibal commented Jan 22, 2022

haxtibal commented Jan 23, 2022

bluetech left a comment

Choose a reason for hiding this comment

haxtibal commented Jan 26, 2022

bluetech commented Jan 27, 2022

bluetech commented Feb 12, 2022

haxtibal commented Feb 19, 2022 • edited Loading

haxtibal commented Feb 20, 2022

nicoddemus commented May 31, 2022

haxtibal commented May 31, 2022

obestwalter commented Jun 20, 2024 • edited Loading

haxtibal Jan 22, 2022 •

edited

Loading

haxtibal commented Feb 19, 2022 •

edited

Loading

obestwalter commented Jun 20, 2024 •

edited

Loading