Skip to content

Conversation

@efiop
Copy link
Contributor

@efiop efiop commented Sep 23, 2023

Existing logic might result in weird fs cache location like .dvc/cache/files/md5/fs, while we really only want .dvc/cache/fs, since it is not an odb cache.

dvc/cachemgr.py Outdated
self._odb["legacy"] = legacy_odb

@property
def fs(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be confusing with fs the filesystem. Maybe file_storage or file_store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also thought about that, but it is a property of cache manager, not particular cache. It is meant to mean fs_storage or fs_cache, since it is a per-fs cache. E.g. we have dvc.cache.local, dvc.cache.repo, dvc.cache.s3(obsoleted) and dvc.cache.fs. I'll rename it to fs_cache then.

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
dvc/cachemgr.py 100.00%
dvc/repo/index.py 100.00%

📢 Thoughts on this report? Let us know!.

self._odb["legacy"] = legacy_odb

@property
def fs_cache(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also could you please write a docstring on what this is being used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added.

Existing logic might result in weird fs cache location like
`.dvc/cache/files/md5/fs`, while we really only want
`.dvc/cache/fs`, since it is not an odb cache.
@efiop efiop merged commit 1fa9f1f into treeverse:main Sep 23, 2023
@efiop efiop self-assigned this Sep 23, 2023
@efiop efiop added the bugfix fixes bug label Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix fixes bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants