Skip to content

Optimization: Avoid quadratic behavior in IdMaker.make_unique_parameterset_ids #14739

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.

Sign up for GitHub

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

Open
NoxiousTab wants to merge 3 commits into
base: main
Choose a base branch
from
+6 −2
Open

Optimization: Avoid quadratic behavior in IdMaker.make_unique_parameterset_ids #14739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06f6de9
Avoid quadratic behavior in IdMaker.make_unique_parameterset_ids
NoxiousTab Jul 20, 2026
1f0cfd8
Add changelog entry for #14739
NoxiousTab Jul 20, 2026
9d5c200
Merge branch 'main' into optimize-idmaker-dedup
NoxiousTab Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/14739.improvement.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
IdMaker.make_unique_parameterset_ids (used to generate ids for @pytest.mark.parametrize) no longer scales quadratically with the number of parameter sets when many of them produce colliding ids, significantly speeding up collection for large parametrized test suites with many duplicate ids.
7 changes: 5 additions & 2 deletions src/_pytest/python.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +930,8 @@ def make_unique_parameterset_ids(self) -> list[str | _HiddenParam]:
"""
resolved_ids = list(self._resolve_ids())
# All IDs must be unique!
if len(resolved_ids) != len(set(resolved_ids)):
all_ids = set(resolved_ids)
if len(resolved_ids) != len(all_ids):
# Record the number of occurrences of each ID.
id_counts = Counter(resolved_ids)

Expand Down Expand Up @@ -963,6 +964,7 @@ def make_unique_parameterset_ids(self) -> list[str | _HiddenParam]:
# Map the ID to its next suffix.
id_suffixes: dict[str, int] = defaultdict(int)
# Suffix non-unique IDs to make them unique.
used_ids = all_ids

@RonnyPfannschmidt RonnyPfannschmidt Jul 21, 2026

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.

used_ids = all_ids is just an alias of the same set object, so mutations below also mutate all_ids. Prefer a single name (used_ids = set(resolved_ids)), and once you add the remaining-count discard logic this alias becomes even more misleading.

for index, id in enumerate(resolved_ids):
if id_counts[id] > 1:
if id is HIDDEN_PARAM:
Expand All @@ -971,10 +973,11 @@ def make_unique_parameterset_ids(self) -> list[str | _HiddenParam]:
if id and id[-1].isdigit():
suffix = "_"
new_id = f"{id}{suffix}{id_suffixes[id]}"
while new_id in set(resolved_ids):
while new_id in used_ids:
id_suffixes[id] += 1
new_id = f"{id}{suffix}{id_suffixes[id]}"
resolved_ids[index] = new_id
used_ids.add(new_id)
Comment on lines 979 to +980

@RonnyPfannschmidt RonnyPfannschmidt Jul 21, 2026

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 only half-updates the parallel set: resolved_ids[index] is replaced, but the old id is never removed from used_ids when it no longer occurs in the list.

That is not just an invariant smell — it changes assigned ids whenever a later candidate equals a fully-renamed-away earlier id.

Simplified counter-example (same algorithm as this loop):

# input
["a10", "a10"] + ["a"] * 11

# main today (rebuild set(resolved_ids) each probe)
["a10_0", "a10_1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a10"]

# this PR (stale "a10" left in used_ids)
["a10_0", "a10_1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a11"]  # <-- last id wrong

After both "a10" entries are renamed, main frees "a10" for the 11th "a". Here "a10" stays reserved forever, so the last id becomes "a11".

Please keep used_ids in sync, e.g. with a remaining-count:

used_ids = set(resolved_ids)
remaining = Counter(resolved_ids)
...
remaining[id] -= 1
if remaining[id] == 0:
    used_ids.discard(id)
resolved_ids[index] = new_id
used_ids.add(new_id)

id_suffixes[id] += 1
assert len(resolved_ids) == len(set(resolved_ids)), (
f"Internal error: {resolved_ids=}"
Expand Down