remote: separate cloud remote and cloud cache classes #4019
Conversation
pmrowla
force-pushed
the
separate-remote-cache
branch
from
61b8acf to
d51da92
Compare
| @property | ||
| def state(self): | ||
| return self.repo.state | ||
|
|
||
| def get(self, md5): |
There was a problem hiding this comment.
Is this function needed? It looks like it was leftover from some old behavior, and is local only (not found in BaseRemote). It's still being tested in tests/func/test_data_cloud but I couldn't find anywhere else in DVC that it's actually used.
| pass | ||
|
|
||
|
|
||
| class CacheMixin: |
There was a problem hiding this comment.
Cloud cache is technically not an extension to cloud remote, since we define remote as the cloud push/pull mirror for a local .dvc MD5 checksum-based cache. However, cloud caches still need to implement the remote methods for size estimation and garbage collection (so that we can display progress during gc).
Originally, I had separate BaseCloud, RemoteMixin, CacheMixin, classes, but it seemed strange to have it like that if every cache class was always going to include the RemoteMixin methods. So for now all the cloud cache classes are implemented as an extension of a cloud remote class + the cache mixin.
pmrowla
force-pushed
the
separate-remote-cache
branch
from
d51da92 to
2a690a4
Compare
|
Will need rebase after #3991 is merged to resolve |
* makes RemoteTree and RepoTree consistent with regard to checksum calculation
* when tree is remote.tree, save will be a move + link operation (same as default existing behavior) * when saving path from a different tree, save will be a copy operation
pmrowla
force-pushed
the
separate-remote-cache
branch
from
2a690a4 to
f3a6a1e
Compare
| @@ -26,21 +35,30 @@ | |||
| ] | |||
|
|
|||
|
|
|||
| def _get(remote_conf): | |||
| for remote in REMOTES: | |||
| def _get(remote_conf, remotes, default): | |||
There was a problem hiding this comment.
@efiop I think cleaning this up to use tree.supported(...) will also require moving scheme and the remaining constants into the tree classes like we discussed. And then once we have single Remote and Cache classes that take a tree as a parameter, we'll be able to get rid of these helper methods.
I think it would be best to leave this as-is for now, and then handle it in the next PR
pmrowla
changed the title
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.
❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Related to #3882
S3Remote,S3Cache,LocalRemote,LocalCache, etc)get_checksumrelated functions are now moved into remote treescache.save()now takes an explicittreeparametertreeisremote.tree, save is done viamove()+link()treeintoremote.tree