Cloud implementation of shutil.copy and shutil.copytree #125

genziano · 2021-02-26T12:32:44Z

Closes #108 and closes #58.

Work in progress.

Three methods of CloudPath implemented:

upload_from, to upload a local file to a cloud file path;
copy, to copy a cloud file to a cloud file or directory path;
copytree, to copy a cloud directory path to another cloud directory path recursively.

Still to implement copy differently, which avoids download / upload when the client is the same.

(I will take care of the tests after we're set with the changes.)

jayqi · 2021-02-27T07:03:17Z

A few thoughts:

I'm not convinced about the shutil-style functions. The multiple cases could either be considered as more convenient or more confusing. I'm leaning that it's more confusing.
- For cloud-to-local, we already CloudPath.download_to which feels more explicit and clear to me.
- For local-to-cloud, I think we should have an upload_from method which is the subject of API for creating a cloud file by uploading it #58
- Local-to-local that just dispatches to shutil feels unnecessary
Then for the cloud-to-cloud, there are actually two cases, which Peter had mentioned in this comment:
- We download the file and reupload it, like what you have.
- If we're within the same cloud provider, we could copy directly without needing to download. I think the way you would check for that case is if the two CloudPath instances share the same client in self.client.

genziano · 2021-02-27T09:04:40Z

A few thoughts:

I'm not convinced about the shutil-style functions. The multiple cases could either be considered as more convenient or more confusing. I'm leaning that it's more confusing.

For cloud-to-local, we already CloudPath.download_to which feels more explicit and clear to me.

For local-to-cloud, I think we should have an upload_from method which is the subject of API for creating a cloud file by uploading it #58

Local-to-local that just dispatches to shutil feels unnecessary

Then for the cloud-to-cloud, there are actually two cases, which Peter had mentioned in this comment:

We download the file and reupload it, like what you have.

If we're within the same cloud provider, we could copy directly without needing to download. I think the way you would check for that case is if the two CloudPath instances share the same client in self.client.

I will move the implementation as methods of CloudPath, also because I couldn't do what I thought because of circular imports.

For the cloud-to-cloud, I was planning indeed to implement differently the case with the same client. And about this, do you have a preferred approach of checking whether the client of src and dst is the same? Checking equality on object level might be too strict.

jayqi · 2021-02-27T09:26:08Z

For the cloud-to-cloud, I was planning indeed to implement differently the case with the same client. And about this, do you have a preferred approach of checking whether the client of src and dst is the same? Checking equality on object level might be too strict.

I think actually that checking for the same instance is the right way to go, i.e., self.client is dst.client. cloudpathlib is implemented in a way that cloud path instances should share references to the same client instance unless explicitly created under a different one.

pjbull · 2021-03-04T05:42:42Z

cloudpathlib/cloudshutil.py

+ """
+ if not src.is_file():
+ raise ValueError('src must be a file.')
+ if isinstance(src, CloudPath) & isinstance(dst, CloudPath):


One little note that all these should be and not &

codecov · 2021-03-04T14:40:27Z

Codecov Report

Merging #125 (541a1b1) into master (a19902e) will decrease coverage by 1.9%.
The diff coverage is 11.5%.

@@           Coverage Diff            @@
##           master    #125     +/-   ##
========================================
- Coverage    91.8%   89.8%   -2.0%     
========================================
  Files          19      19             
  Lines        1050    1076     +26     
========================================
+ Hits          964     967      +3     
- Misses         86     109     +23

Impacted Files	Coverage Δ
cloudpathlib/cloudpath.py	`84.8% <11.5%> (-5.4%)`	⬇️

pjbull · 2021-03-04T23:49:05Z

cloudpathlib/cloudpath.py

+ )
+ temp_file = self._local
+ temp_file.parent.mkdir(parents=True)
+ self.download_to(temp_file)


Instead of download to a temp_file, you can do self.fspath which will use the cached version if it is downloaded and download it to the cache if not. So the lines below can just use that. For example destination.upload_from(self.fspath)

pjbull · 2021-03-04T23:51:32Z

cloudpathlib/cloudpath.py

+ raise ValueError(
+ f"Origin path {self} must be a directory. To copy a single file use the method copy."
+ )
+ if not destination.is_dir():


This check might require that destination already exists, but I don't think that should be a requirement. Maybe use destination.exists() and destination.is_file() instead?

Agreed. is_dir will return False if destination does not exist. And, shutil.copytree will create the destination directory, so we should mirror that behavior.

pjbull · 2021-03-13T19:07:22Z

Hi @genziano, seems like this is heading in the right direction—excited to get it merged!

Anything that we can help with in terms of implementation questions or writing tests?

genziano · 2021-03-16T18:36:17Z

Hi @genziano, seems like this is heading in the right direction—excited to get it merged!

Anything that we can help with in terms of implementation questions or writing tests?

Hi @pjbull! I have unfortunately been busy these last weeks, but next week I will work on finishing it up, and hopefully picking up some new issue.

Unrelated to this PR, I have a question: do you guys have some examples of usage with Azure blob storage? I'm not sure about how to create paths pointing to azure blob accounts. I'd also be happy to include some examples if needed!

pjbull · 2021-03-16T19:01:11Z

do you guys have some examples of usage with Azure blob storage

Is something like this what you mean or also including the authentication/credential flow (which is in the docs)

from cloudpathlib import CloudPath, AzureBlobPath

# generic version starting with az:// is dispatched to AzureBlobPath
az_path = CloudPath("az://container/directory/file.txt")

# explicit version
az_path = AzureBlobPath("az://container/directory/file.txt")

genziano · 2021-03-16T19:49:13Z

do you guys have some examples of usage with Azure blob storage

Is something like this what you mean or also including the authentication/credential flow (which is in the docs)
from cloudpathlib import CloudPath, AzureBlobPath

# generic version starting with az:// is dispatched to AzureBlobPath
az_path = CloudPath("az://container/directory/file.txt")

# explicit version
az_path = AzureBlobPath("az://container/directory/file.txt")

Yes thanks, exactly that. I didn't figure out that the account name was just part of the connection string.

BenAsaf · 2021-04-12T04:43:15Z

Hello!
Any news on this? I am looking forward to this PR :)
Can I assist in anyway?

* Cloud implementation of shutil.copy and shutil.copytree (#125) * Draft of cloudpathlib.copy, that replicates shutil.copy * Import only PathLike from os * Reuse _local_to_cloud_copy for cached files * try to use the intended cached _local version * Update cloudshutil.py * Implemented the CloudPath methods: upload_from, copy, copytree * Add docstrings * Fix indentation * fix unnecessary imports * Fix typing issues * Allow CloudPath.copy to a directory as well * Add remove_src option to enable copy * update copy, copytree, upload_from * Fixes for bugs from writing tests * Tests for copy, copytree, upload * Mypy checks * Add sleeps to ensure write times in tests * Update functions table * extra sleep time for live tests * Address code review comments * Add chagelog updates for release Co-authored-by: Alessandro Gentile <alemaudit@gmail.com>

genziano added 4 commits February 26, 2021 13:25

Draft of cloudpathlib.copy, that replicates shutil.copy

efe43b9

Import only PathLike from os

c4428f6

Reuse _local_to_cloud_copy for cached files

cfcafe6

try to use the intended cached _local version

cd22e5d

pjbull reviewed Mar 4, 2021

View reviewed changes

genziano added 7 commits March 4, 2021 14:38

Update cloudshutil.py

7bbf5d4

Implemented the CloudPath methods: upload_from, copy, copytree

893d271

Merge with origin

b9c7f64

Add docstrings

613c84d

Fix indentation

25df928

fix unnecessary imports

bff6408

Fix typing issues

b6fc629

Allow CloudPath.copy to a directory as well

85055e1

pjbull reviewed Mar 4, 2021

View reviewed changes

pjbull changed the base branch from master to 108-copy-upload April 21, 2021 05:43

pjbull marked this pull request as ready for review April 21, 2021 05:44

pjbull merged commit 26337a9 into drivendataorg:108-copy-upload Apr 21, 2021

pjbull mentioned this pull request Apr 21, 2021

Cloud implementation of shutil.copy and shutil.copytree #142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloud implementation of shutil.copy and shutil.copytree #125

Cloud implementation of shutil.copy and shutil.copytree #125

genziano commented Feb 26, 2021 •

edited

Loading

jayqi commented Feb 27, 2021 •

edited

Loading

genziano commented Feb 27, 2021

jayqi commented Feb 27, 2021

pjbull Mar 4, 2021

codecov bot commented Mar 4, 2021 •

edited

Loading

pjbull Mar 4, 2021

pjbull Mar 4, 2021

jayqi Mar 13, 2021

pjbull commented Mar 13, 2021

genziano commented Mar 16, 2021

pjbull commented Mar 16, 2021

genziano commented Mar 16, 2021

BenAsaf commented Apr 12, 2021

Cloud implementation of shutil.copy and shutil.copytree #125

Cloud implementation of shutil.copy and shutil.copytree #125

Conversation

genziano commented Feb 26, 2021 • edited Loading

jayqi commented Feb 27, 2021 • edited Loading

genziano commented Feb 27, 2021

jayqi commented Feb 27, 2021

pjbull Mar 4, 2021

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2021 • edited Loading

Codecov Report

pjbull Mar 4, 2021

Choose a reason for hiding this comment

pjbull Mar 4, 2021

Choose a reason for hiding this comment

jayqi Mar 13, 2021

Choose a reason for hiding this comment

pjbull commented Mar 13, 2021

genziano commented Mar 16, 2021

pjbull commented Mar 16, 2021

genziano commented Mar 16, 2021

BenAsaf commented Apr 12, 2021

genziano commented Feb 26, 2021 •

edited

Loading

jayqi commented Feb 27, 2021 •

edited

Loading

codecov bot commented Mar 4, 2021 •

edited

Loading