Skip to content

Improvement: #13420 - Update logic of nodes._check_initialpaths_for_relpath #13448

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.

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

Conversation

sashko1988
Copy link
Contributor

@sashko1988 sashko1988 commented May 28, 2025

Updates logic of nodes._check_initialpaths_for_relpath

An even better fix for #13420 and this discussion - #13413

I still wasn't quite happy with the performance.

So after close examination of what nodes._check_initialpaths_for_relpath does, I came up with the PR changes.

Previous logic:

  • Iterate over the set of initial_paths.
  • try to find common path of given path and current iteration initial path
  • if common path exists - compare it with current iteration initial path
  • if common path is the same as the current iteration initial path, find path relative to given path and return

Instead, I propose the following:

  • Check if the given path is in initial_paths.
    • if it is, then we know that relative_to will return ".", and function should return ""
  • check failed, check if any parent of the given path is in initial_paths. If it is, then return the relative path of the given path and that parent.

Performance changes for collecting 1000 tests with stucture described in #13420 and #13413 (times are almost the same for main and 8.3.x branches):

Base

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
     5821    2.591    0.000   83.172    0.014 nodes.py:547(_check_initialpaths_for_relpath)

With improved logic

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
     5821    0.020    0.000    0.337    0.000 nodes.py:547(_check_initialpaths_for_relpath)

PS Couple of questions:

  1. Should I create a separate issue for that PR or associate Slow collection time when tests are not in a relative folder to the current working folder #13420 with it?
  2. Should I create a changelog entry?
  3. Should I add myself to the authors?)

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Clever

Let's associate the pr id and mention further improvement

update AUTHORS
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label May 28, 2025
@sashko1988 sashko1988 changed the title Update logic of nodes._check_initialpaths_for_relpath Improvement: #13420 - Update logic of nodes._check_initialpaths_for_relpath May 28, 2025
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Nice catch! Thanks!

@nicoddemus nicoddemus dismissed RonnyPfannschmidt’s stale review May 28, 2025 21:22

Changelog updated as requested.

@nicoddemus
Copy link
Member

Should I add myself to the authors?)

Yes, you are welcome to. 👍

@sashko1988
Copy link
Contributor Author

@nicoddemus thanks for editing the changelog entry!

@sashko1988
Copy link
Contributor Author

@nicoddemus @RonnyPfannschmidt
Can it also be backported to 8.3.x?

@nicoddemus nicoddemus merged commit bd6877e into pytest-dev:main May 29, 2025
28 checks passed
Copy link

patchback bot commented May 29, 2025

Backport to 8.3.x: 💚 backport PR created

✅ Backport PR branch: patchback/backports/8.3.x/bd6877e5874b50ee57d0f63b342a67298ee9a1c3/pr-13448

Backported as #13451

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request May 29, 2025
Use `Path` facilities to greatly improve performance of `nodes._check_initialpaths_for_relpath` (see #13448 for performance comparison).

Related: #13420, #13413.

---------

Co-authored-by: Bruno Oliveira <[email protected]>
(cherry picked from commit bd6877e)
nicoddemus added a commit that referenced this pull request May 29, 2025
…13451)

Use `Path` facilities to greatly improve performance of `nodes._check_initialpaths_for_relpath` (see #13448 for performance comparison).

Related: #13420, #13413.

---------


(cherry picked from commit bd6877e)

Co-authored-by: Sashko <[email protected]>
Co-authored-by: Bruno Oliveira <[email protected]>
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.

3 participants