Skip to content

Conversation

@pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Jun 8, 2020

  • ❗ 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. πŸ™

  • remote.save() now takes an explicit tree parameter
    • When tree is remote.tree, save is done via move() + link()
    • For all other trees, save is done by copying object from tree into remote.tree

pmrowla added 2 commits June 8, 2020 16:12
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

So we still don't have a cache/remote separation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@efiop Not yet. I'm still thinking about the best way to go about that particular separation.

The "remote" functions like push/pull/status (which are currently in LocalRemote) are really functions for syncing the contents of two caches. So I've been wondering if they should just be abstracted into functions for syncing from "this/self" cache to/from any other cache, and then moved into BaseRemote?

And then the remote classes could still be renamed to Cache or RemoteCache (or something else along those lines), but there wouldn't be any further separation into new classes (other than the tree methods).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that we would still have the external output/dependency use case to consider, but it seems like in practice, that is still just a situation where we would have two "s3 caches"

  • one which is used as the typical "dvc remote" for syncing local cache <-> s3
  • and the second which is used as the s3 external out cache

@pmrowla pmrowla force-pushed the remote-save-tree-param branch from ed0b8e7 to 0466227 Compare June 8, 2020 09:12
@pmrowla pmrowla self-assigned this Jun 8, 2020
@pmrowla pmrowla closed this Jun 11, 2020
@pmrowla
Copy link
Contributor Author

pmrowla commented Jun 11, 2020

closed in favor of #4019

@pmrowla pmrowla deleted the remote-save-tree-param branch June 11, 2020 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants