Cache file names should include file name extension

[aschuh-hf]

At the moment, names of files in the DVC cache have no file name extension, unless it is a special .dir text file. This poses issues when using symbolic links from the workspace to the DVC cache and the use of pathlib.Path.resolve(), for example, when the consuming program determines based on the file name extension the file format (e.g., image file format, PNG, JPEG). This would be solved by keeping the filename extension when moving files to the DVC cache (e.g., {cache}/{hash[:2]}/{hash[2:}{ext}, where ext=".png"). Because such change would invalidate existing caches, maybe consider adding this as a option in .dvc/config that a new project can opt-in to.

As a concrete example of an image file reader which requires a proper file name extension, see SimpleITK.ReadImage(), which is more common in medical applications than computer vision.

[efiop]

Hi @aschuh-hf .

Our cache is content-based storage, and shouldn't care about workspace file extensions at all. Even if we assume that it shoukd - two files with the same content might have different extensions or one could not have it at all, which means that this doesn't fit at all into our architecture.

In this particular case, would using hardlinks be an option for you?

I wonder if ReadImage() is doing that for some specific reason? Seems like it could be logical to use extension of the symlink name, instead of fully resolving it (parents need to be resolved, no doubt, but not so sure about the filename).

[aschuh-hf]

I think it's true that there are workarounds, granted that one can modify the code that resolves symbolic links. As a function such as SimpleITK.ReadImage only takes a single path string argument, it would not be possible to both have a resolved path without symbolic links (i.e., the path of the file in the DVC cache) and provide the extension of the original symbolic link at the same time [EDIT: This isn't quite right, in this instance, ReadImage also accepts a imageIO argument; but again this requires that the file name extension is still known at the point where this function is called]. In this particular case, the user code would need to either avoid pathlib.Path.resolve() altogether (including any path simplifications, i.e., os.path.normpath) or use a concrete image file reader (e.g., PNGImageIO).

But if such call to an IO function is taking place in a third-party dependency which is developed without these considerations specific to the use of DVC in mind, I may not have control over this. Personally, I for now can work around this as above, but I thought it was a limitation that the choice of how DVC links from workspace to it's content cache influences how I have to write my Python program.

[efiop]

@aschuh-hf Have you tried passing an unresolved path to the ReadImage?

[schuhschuh]

That works of course. But my point is that because of how the DVC cache is structured and linked using symbolic links, one must be careful about resolving input file paths (e.g., constructed from PWD, params.yaml entry etc.) prior to such operation. Furthermore, the code that resolves an input file path may be situated at a very different location than the code that uses the file to actually read the input from disk. If the file name extension was preserved (given that the DVC cache is indeed file based), this would not be the case. ReadImage was only an example to illustrate my point.

[aschuh-hf]

I just thought I'd bring it up to see if there was a way to support an option to enable the use of file name extension also for the file-based DVC cache. If this doesn't fit the design of DVC, OK not to consider. The issue can either be closed without change, or maybe a comment be added to the documentation regarding limitations of using symbolic links.

[efiop]

@schuhschuh If that works, then there is no issue, as that's how the symlink conventions work and SimpleITK is properly processing symlinks.

Symlinks are not guaranteed to have matching link and target names, which extension is a part of, so this is not really a DVC limitation, this is a limitation of the code that handles the symlink. This is a pretty common problem though, people (me included) tend to not process them correctly, improperly or prematurely resolving them, using stat() instead of lstat() when they care about the symlink itself, etc 🙁 That's one of the reasons why we also support hardlinks, as they don't break code that doesn't handle symlinks properly. I'm sure you have very good reasons to use resolve(), but be aware that it might break in other symlink scenarios unrelated to dvc, so you might need to consider handling them as other libraries do.

[schuhschuh]

Ok, that's fine. When I avoid resolving symbolic links it's working fine.

Re: hardlinks, I presume these are not always a possibility, i.e. when the cache and workspace are not on the same logical (?) FS.

[efiop]

Re: hardlinks, I presume these are not always a possibility, i.e. when the cache and workspace are not on the same logical (?) FS.

Correct, hardlinks only work within the same filesystem.

Closing for now. Thanks for the feedback! 🙏