Skip to content
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

Skip repeated directories in PATH when searching for Python interpreters #12367

Merged
merged 3 commits into from
Apr 3, 2025

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Mar 21, 2025

Closes #12302

The change is visible in this commit.

@zanieb zanieb added the enhancement New feature or improvement to existing functionality label Mar 21, 2025
which::which_in_global("python.bat", Some(&dir_clone))
// If we cannot determine if the directory is unique, we'll assume it is
.unwrap_or(true)
.then(|| {
Copy link
Member Author

Choose a reason for hiding this comment

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

@charliermarsh
Copy link
Member

Should this be implemented by de-duping the interpreters rather than the parent directories? I assume not, but it'd be good to note why.

@zanieb
Copy link
Member Author

zanieb commented Apr 3, 2025

Should this be implemented by de-duping the interpreters rather than the parent directories?

We probably could but I'm not sure what it gets us. We'd spend more time collecting interpreters we've already seen. We could dedupe interpreter paths instead of parent directories but that also just adds redundant directory item enumeration.

@charliermarsh
Copy link
Member

We could dedupe interpreter paths instead of parent directories but that also just adds redundant directory item enumeration.

I think that cost of that will be really low compared to the cost of querying the interpreters. But is it even better? Do we want to show symlinks of adjacent Python interpreters (like python vs python3 in the same directory)? I can't rememeber.

@zanieb
Copy link
Member Author

zanieb commented Apr 3, 2025

I think that cost of that will be really low compared to the cost of querying the interpreters.

I agree. I don't see why it'd be preferable though.

But is it even better? Do we want to show symlinks of adjacent Python interpreters (like python vs python3 in the same directory)?

Yes, I think it's important we can represent this. There are a couple reasons off the top of my head...

  • It's meaningful to show that python and python3 are both available on the PATH and invoke the same interpreter
  • The name of the executable is relevant for some downstream logic around "default" versions, i.e., have you opted into free-threaded Python by installing it as python3?

We could deduplicate those later, e.g., during the uv python list display (we already do some deduplication there). However, during discovery it's important to avoid filtering too aggressively.

@zanieb zanieb marked this pull request as ready for review April 3, 2025 14:44
Copy link

codspeed-hq bot commented Apr 3, 2025

CodSpeed Performance Report

Merging #12367 will degrade performances by 10.19%

Comparing zb/python-seen (ef6a569) with main (be3d5df)

Summary

❌ 1 regressions
✅ 13 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
wheelname_tag_compatibility[flyte-short-incompatible] 771.4 ns 858.9 ns -10.19%

@zanieb zanieb merged commit ac145d8 into main Apr 3, 2025
98 of 99 checks passed
@zanieb zanieb deleted the zb/python-seen branch April 3, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python list: don't list system interpreters twice when directories are symlinked
2 participants