Skip to content

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 efiop and shcheklein 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
@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
Contributor

@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
Contributor

@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.

@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
Contributor

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 treeverse: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