Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gdrive: download: stream & add progress #3722

Merged
merged 6 commits into from
May 6, 2020

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented May 2, 2020

@casperdcl casperdcl added refactoring Factoring and re-factoring ui user interface / interaction performance improvement over resource / time consuming tasks research labels May 2, 2020
@casperdcl casperdcl requested review from shcheklein and efiop May 2, 2020 18:43
@casperdcl casperdcl self-assigned this May 2, 2020
@casperdcl casperdcl changed the title Progress gdrive gdrive: progress for downloads May 2, 2020
dvc/remote/gdrive.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #3722 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3722   +/-   ##
=======================================
  Coverage   90.80%   90.80%           
=======================================
  Files         158      158           
  Lines       10582    10582           
=======================================
  Hits         9609     9609           
  Misses        973      973           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d00bd4d...58490e0. Read the comment docs.

@casperdcl casperdcl marked this pull request as ready for review May 5, 2020 20:58
@casperdcl
Copy link
Contributor Author

P.S. @shcheklein I don't think we need to bother about GetContentFile(remove_bom=True) since we never Upload({"convert": True}).

@shcheklein
Copy link
Member

@casperdcl yep, that's why I was not that worried about bom in the first place (e.g. performance).

@casperdcl casperdcl changed the title gdrive: progress for downloads gdrive: download: stream & add progress May 5, 2020
@casperdcl casperdcl requested a review from efiop May 5, 2020 21:17
@casperdcl
Copy link
Contributor Author

Also #2865 says

  • and update docs

for the file streaming bit... What docs?

@shcheklein
Copy link
Member

@casperdcl yeah, this one doesn't solve the API streaming. It is the next level of avoiding writing stuff to the disk - dcv.api.open and dvc get. E.g. for S3 they call get_signed_url internally, instead of _download.

dvc/remote/gdrive.py Outdated Show resolved Hide resolved
@casperdcl
Copy link
Contributor Author

casperdcl commented May 5, 2020

yeah, this one doesn't solve the API streaming.

ah I see

@shcheklein
Copy link
Member

Patches like this:

Screen Shot 2020-05-05 at 3 31 49 PM

  • less (?) API calls + UI improved + download faster ...

just warm my heart :) Less code we have better I sleep. Thanks 🙏

@casperdcl
Copy link
Contributor Author

casperdcl commented May 5, 2020

yes just pushed a final quick update (because of the deferred total) to ensure consistent formatting so a tiny bit more loc :)

+11
-21

Pretty sure there's at least one (if not 3) fewer API calls. #2511 would tell.

@casperdcl
Copy link
Contributor Author

also please don't merge until conda-forge/pydrive2-feedstock#10 (which doesn't exist yet)

@casperdcl
Copy link
Contributor Author

ok good to go

@efiop efiop merged commit 26e2032 into iterative:master May 6, 2020
@efiop
Copy link
Contributor

efiop commented May 6, 2020

Thank you @casperdcl ! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance improvement over resource / time consuming tasks refactoring Factoring and re-factoring research ui user interface / interaction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants