[internal] Use fast_relpath_optional for source root calculation in local_dists. #13091
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| source.startswith(source_root) | ||
| and os.path.relpath(source, source_root) in provided_files | ||
| ): | ||
| source_relpath = fast_relpath_optional(source, source_root) |
There was a problem hiding this comment.
It might be good if source_roots were non-str / non-PathLike objects so attempts to use os APIs failed, forcing you to convert them to a PathLike 1st. That might be enough pause to see new, perf appropriate API's hung off the source root type.
There was a problem hiding this comment.
Mm... yea. I think that for a bit there we thought that Path/PurePath were going to be this better API, but it unfortunately seems that that isn't the case.
The challenge is always how widespread these APIs are: there is a huge surface area in the stdlib that takes either str/Path, so any custom path type has a high bar to be worth the extra boilerplate of conversions at the boundary. And we don't have Deref like on the Rust side.
But yea, maybe if it were isolated to a SourceRoot type... although that limits the upside of the custom type as well.
This usage of
os.path.relpathwas showing up in profiles: it's a notoriously expensive function due to its support for relativizing non-contained paths, and use ofos.getcwd(which releases the GIL to execute a syscall, and then needs to re-acquire it).Switch to
fast_relpath_optional.[ci skip-rust]
[ci skip-build-wheels]