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

Use of os.path.realpath() leads to heavy metadata IOPS on some systems #8543

Closed
obilaniu opened this issue Nov 8, 2024 · 4 comments · Fixed by #8545
Closed

Use of os.path.realpath() leads to heavy metadata IOPS on some systems #8543

obilaniu opened this issue Nov 8, 2024 · 4 comments · Fixed by #8545

Comments

@obilaniu
Copy link

obilaniu commented Nov 8, 2024

The Pillow functions Image.open() and Image.save(), among others, use os.path.realpath() on the input path to transform it to a symlink-free canonical path.

There is no reliance on this path's "realness" afterwards that is obvious to me, and realpath() requires an lstat() system call on every single path segment from / on down to the file in question (or more, if symlinks are detected along the way). On clusters with distributed filesystems such as BeeGFS, this leads and is seen as a multiplication (by a factor greater than or equal to the depth of the image file within the hierarchy) of the number of metadata I/O operations, and causes DDoS of the metadata servers for those filesystems.

Pillow is heavily utilized in AI workloads with image datasets and data-loaders, a popular usecase, making Pillow's use of realpath() especially painful. PyTorch's ImageFolder is the classic example.

Please reconsider all uses of realpath() in Pillow. Preferentially remove it entirely. In the alternative that you absolutely need the last segment of the path to be translated to acquire a "true" file-extension, consider readlink().

@atong01
Copy link

atong01 commented Nov 8, 2024

This is a significant issue for us and can lead to cluster-wide slowdown. Would be great to fix!

@radarhere
Copy link
Member

The first use of os.path.realpath came from #7750 (comment)

@AlexWaygood since that was your comment, did you have any thoughts on this?

@AlexWaygood
Copy link

The first use of os.path.realpath came from #7750 (comment)

Hmm, it looks like prior to that you were using pathlib.Path.resolve(), which just uses os.path.realpath() under the hood: https://github.com/python/cpython/blob/6293d00e7201f3f28b1f4512e57dc4f03855cabd/Lib/pathlib/_local.py#L728. So I doubt this problem truly started with the changes related to that issue.

I have little opinion on whether realpath() is the most appropriate function for you to use here! IIRC, my suggestion aimed to be a fairly direct translation of the existing code so that it would work with arbitrary PathLike objects rather than just pathlib.Path :-)

@radarhere
Copy link
Member

Ok, looking for the source of resolve(), it was #1372. There's no indication why it was added there 9 years ago.

I've created #8545 to remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants