Skip to content

Avoid unnecessary descriptor reads with raising=False - #15091

Open
houtinghang wants to merge 5 commits into
pytest-dev:mainfrom
houtinghang:codex/monkeypatch-descriptor-10646
Open

houtinghang wants to merge 5 commits into
pytest-dev:mainfrom
houtinghang:codex/monkeypatch-descriptor-10646

Conversation

@houtinghang

@houtinghang houtinghang commented Sep 23, 2026 •

Copy link
Copy Markdown

With raising=False, patching a class descriptor currently calls its __get__ once in setattr and twice in delattr. A descriptor that raises can prevent the patch altogether. This change uses static lookup for class and non-data descriptors, while keeping normal lookup for instance data descriptors whose values are needed by undo().

Refs #10646.

The change keeps raising=True behavior and falls back to dynamic lookup for attributes supplied by __getattr__. It does not promise side-effect-free access to instance properties or slots. Regression tests cover descriptor side effects and exceptions, restoration, inherited class attributes, dynamic attributes, properties, and slots.

Validation on Windows / Python 3.13.9:

  • Before the fix: five new regression cases failed.
  • Full suite: 4542 passed, 160 skipped, 13 xfailed, 1 xpassed, with 2 warnings (exit code 0).
  • All applicable pre-commit hooks passed, including mypy.

Full-suite command (UTF-8 mode and the virtualenv Scripts directory on PATH):

$env:PYTHONUTF8 = '1'
$env:PYTHONIOENCODING = 'utf-8'
$env:PYTHONWARNDEFAULTENCODING = '1'
python -m pytest -q -n 4 --basetemp=C:/Temp/pt10646-full-923

The first local runs exposed subprocess encoding errors and a long-path bytecode-cache failure. UTF-8 mode removed the encoding errors; a short temporary path resolved the cache test. The unmodified base also failed that cache test with the long path. The XPASS is the existing test_make_numbered_dir_multiprocess_safe case (#11603).

AI assistance: OpenAI Codex helped prepare the implementation, tests, and this description under my direction; it is credited in the commit. The upstream CI matrix passed on commit 9cdb28e.

CI follow-up:

  • Exercise descriptor reads before patching, including their exception paths, and the missing dynamic-attribute path. These positive controls verify the original behavior and cover the newly added test branches.
  • Keep one narrowly matched NumPy/Cython RuntimeWarning visible instead of treating it as an error. The PyPy job failed in test_idval_hypothesis while importing numpy.random; the same failure reproduces with standalone import numpy.random under -W error, without pytest installed (PyPy 8.0.0 / Python 3.11.16, NumPy 2.4.6).
  • With that warning filter, the previously failing Hypothesis case plus the monkeypatch suite pass on that PyPy runtime: 58 passed, 1 skipped. Applicable pre-commit checks also pass.

Follow-up correction: the first PyPy focused run did not preload NumPy and therefore missed the failing Hypothesis path. A fresh-process reproduction which imports NumPy before running test_idval_hypothesis fails with the original filter. Cython attributes this warning to <frozen importlib._bootstrap>, so the NumPy module restriction was removed while retaining the specific message and RuntimeWarning category. The same reproduction now passes with the warning displayed (1 passed, 1 warning).

Final CI validation on 9cdb28e0e405ad900355159ceffddf50050c1a08: all 36 PR checks passed, including the final check job, the full OS/Python matrix, and Codecov patch coverage (100%). The PyPy full suite reports 4601 passed, 102 skipped, 12 xfailed, and 1 xpassed. Run: https://github.com/pytest-dev/pytest/actions/runs/35881114712

Read class and non-data descriptors statically before patching so their
__get__ methods are not invoked merely to save the old attribute.
Preserve dynamic lookup and instance data descriptor values for undo.

Refs pytest-dev#10646

Co-authored-by: OpenAI Codex <noreply@openai.com>
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Sep 23, 2026
houtinghang and others added 3 commits September 23, 2026 23:00
Validate the descriptors and dynamic fallback directly before asserting
that monkeypatch does not execute them. This covers the test code itself
without excluding meaningful branches from coverage.

Co-authored-by: OpenAI Codex <noreply@openai.com>
Preserve the warning while avoiding an unrelated Hypothesis failure on NumPy 2.4.6 and PyPy 8.0.0.

Co-authored-by: OpenAI Codex <noreply@openai.com>
Reproduce with NumPy loaded before Hypothesis: the warning is attributed to importlib, so matching numpy.random misses it. Retain the exact warning message and category.

Co-authored-by: OpenAI Codex <noreply@openai.com>

@MateehUllah MateehUllah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the descriptor lookup changes and regression coverage. Using static lookup for class and non-data descriptors when raising=False avoids invoking descriptor code merely to preserve undo state, while instance data descriptors still use normal lookup because their current value must be restored through the setter. The fallback for dynamically provided attributes preserves getattr behavior, and the MRO-based data-descriptor check correctly handles inherited descriptors. The tests cover class descriptors, instance non-data descriptors, properties, slots, inherited attributes, dynamic attributes, both operations, and undo behavior. I did not find a blocking correctness issue.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants