gdrive: add open #3916

casperdcl · 2020-05-30T22:44:36Z

Add open() for gdrive remote
update docs
fixes api: support streaming from Google Drive remotes #3408
related Google Drive support further enhancements #2865
related "AttributeError: NoneType object has no attribute 'wdir'" on dvc get-url call #3897
depends add files.GetContentIOBuffer PyDrive2#39
- depends pydrive2 v1.4.14 conda-forge/pydrive2-feedstock#13
~~depends add len(MediaIoReadable) PyDrive2#40~~
tests in https://github.com/iterative/dvc/blob/1.0.0a6/tests/func/test_api.py#L60
- fix credentials https://travis-ci.com/github/iterative/dvc/jobs/341956240
- rebase onto current master

dvc/dependency/__init__.py

shcheklein · 2020-05-31T02:10:43Z

dvc/remote/gdrive.py

@@ -393,6 +396,23 @@ def _gdrive_download_file(
        ) as pbar:
            gdrive_file.GetContentFile(to_file, callback=pbar.update_to)

+    @contextmanager


just to double check - do we properly pass close down to the PyDrive?

pydrive2 doesn't expose any close() methods anywhere. I think the underlying httplib2 closes connections before raising errors; so no special handling needed.

it should close if just return from the block ... we should check the logic carefully here

upstream issue: iterative/PyDrive2#44

dvc/remote/gdrive.py

tests/func/test_api.py

casperdcl · 2020-05-31T20:01:07Z

hmm tests failing with

ERROR: failed to push data to the cloud - GDrive remote auth failed with credentials in 'GDRIVE_CREDENTIALS_DATA'.
Backup first, remove of fix them, and run DVC again.
It should do auth again and refresh the credentials.

https://travis-ci.com/github/iterative/dvc/jobs/341956240

codecov · 2020-05-31T20:36:19Z

Codecov Report

Merging #3916 into master will decrease coverage by 0.18%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master    #3916      +/-   ##
==========================================
- Coverage   92.32%   92.13%   -0.19%     
==========================================
  Files         160      161       +1     
  Lines       11107    11121      +14     
==========================================
- Hits        10254    10246       -8     
- Misses        853      875      +22

Impacted Files	Coverage Δ
dvc/utils/stream.py	`85.71% <85.71%> (ø)`
dvc/exceptions.py	`96.99% <100.00%> (+0.02%)`	⬆️
dvc/remote/gdrive.py	`81.91% <100.00%> (+0.73%)`	⬆️
dvc/utils/http.py	`95.34% <100.00%> (+4.04%)`	⬆️
dvc/system.py	`79.66% <0.00%> (-9.33%)`	⬇️
dvc/remote/ssh/connection.py	`83.17% <0.00%> (-4.33%)`	⬇️
dvc/analytics.py	`95.06% <0.00%> (-1.24%)`	⬇️
dvc/remote/local.py	`95.52% <0.00%> (-0.50%)`	⬇️
dvc/repo/__init__.py	`97.59% <0.00%> (+0.30%)`	⬆️

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 5efc3bb...8ae3016. Read the comment docs.

shcheklein · 2020-05-31T21:19:49Z

tests/remotes.py

@@ -141,10 +142,13 @@ class GDrive:
    def should_test():
        return os.getenv(GDriveRemote.GDRIVE_CREDENTIALS_DATA) is not None

-    def get_url(self):
-        if not getattr(self, "_remote_url", None):


removing this will break TestGDriveRemoteCLI most likely

in general I believe we should probably introduce caching of get_url in for other remotes as well - it is strange to return different values within one test

@shcheklein It actually shouldn't, because other remotes do that too and just reuse the results. Need to check maybe there is something wrong or special about gdrive...

@efiop I think they just call it once, in case of GDrive it just happened that we need it in multiple places. And in general, it's def not a good and safe interface - this value should be cached one way or another and stay the same during the test.

shcheklein · 2020-05-31T21:24:38Z

tests/func/test_api.py


-remote_params = [S3, GCP, Azure, OSS, SSH, HDFS]
+remote_params = [S3, GCP, Azure, GDrive, OSS, SSH, HDFS]


we have a lot of tests for functions besides open here - do we expect all of them to pass? we'll need to implement more stuff then

@shcheklein All the tests should work, there is nothing new required from GdriveRemotes. There might be some issues with Gdrive itself though (creds or something), that differentiates it from other remotes.

Even it works out of the box, some things do not make much sense - e.g. what will dvc.api.url return for the object? Some URL that no tool (except DVC) can work with?

Re credentials - should be fine (we use an env var to set them, so I would expect external repo to pick it up as well). As I mentioned though, GDrive remote requires some special setup (e.g. it does not create a directory for you).

regarding get_url, I think that's how the value should look like:

(id= is the file ID that we can get via API)

@shcheklein is this comment outdated since rebasing on master? Or does tests/remotes.py still need updating?

@casperdcl Could do that, sure. Will require some work to make it do that without first passing it a dvc instance.

well the only other way I see is to rework the current pytest fixture (currently it hides the GDrive object so there's no way to call create_dir)

@casperdcl Both work. Choose the easiest one, we'll be revisiting this later anyway.

tried a simple change along the same lines as setup_remote but still failing with the same credentials error https://travis-ci.com/github/iterative/dvc/jobs/348374056#L7952

@casperdcl does it work locally with your creds?

This makes `GDrive.get_url` behaviour consistent with the rest of the test remotes. Related to #3916

efiop · 2020-06-04T01:04:38Z

Adjusted GDrive to have a create_dir method that is needed before you can use GDrive remote to push to it. Need to adjust those remote test classes to call it for the setup. @casperdcl could you take a look?

Fixes iterative#3408 Related iterative#2865 Fixes iterative#3897

This reverts commit fd33326.

tests/func/test_api.py

casperdcl · 2020-06-13T18:17:17Z

tests/func/test_api.py

@@ -72,6 +122,7 @@ def test_open(remote_url, tmp_dir, dvc):
 @pytest.mark.parametrize("remote_url", all_remote_params, indirect=True)
 def test_open_external(remote_url, erepo_dir, setup_remote):
    setup_remote(erepo_dir.dvc, url=remote_url)
+    ensure_dir_scm(erepo_dir.dvc, remote_url)


@efiop I've added ensure_dir_scm here based off setup_remote.

Using ensure_dir instead of ensure_dir_scm, there's
ERROR: configuration error - config file error: remote 'upstream' doesn't exists.

Using run_dvc("remote", "add", ...) results in
ERROR: configuration error - config file error: remote 'upstream' already exists. Use -f|--force to overwrite it.

dvc/exceptions.py

efiop · 2020-06-13T20:53:46Z

Thanks @casperdcl ! For the record: s3 test is failing for unrelated reasons.

efiop · 2020-06-13T20:54:18Z

@casperdcl Please don't forget about the docs 🙂

casperdcl added enhancement Enhances DVC refactoring Factoring and re-factoring ui user interface / interaction research labels May 30, 2020

casperdcl requested review from shcheklein and efiop May 30, 2020 22:44

casperdcl self-assigned this May 30, 2020

restyled-io bot mentioned this pull request May 30, 2020

Restyle gdrive: add open #3917

Closed

casperdcl force-pushed the gdrive-stream branch from 135a8d5 to 68cb7e6 Compare May 30, 2020 23:36

shcheklein reviewed May 31, 2020

View reviewed changes

dvc/dependency/__init__.py Outdated Show resolved Hide resolved

shcheklein reviewed May 31, 2020

View reviewed changes

dvc/remote/gdrive.py Show resolved Hide resolved

efiop reviewed May 31, 2020

View reviewed changes

tests/func/test_api.py Outdated Show resolved Hide resolved

efiop reviewed May 31, 2020

View reviewed changes

tests/func/test_api.py Outdated Show resolved Hide resolved

shcheklein reviewed May 31, 2020

View reviewed changes

casperdcl mentioned this pull request May 31, 2020

api: support streaming from Google Drive remotes #3408

Closed

1 task

efiop added a commit that referenced this pull request Jun 3, 2020

tests: don't cache gdrive url

7528ee1

This makes `GDrive.get_url` behaviour consistent with the rest of the test remotes. Related to #3916

efiop added a commit that referenced this pull request Jun 4, 2020

tests: don't cache gdrive url (#3941)

a65c798

This makes `GDrive.get_url` behaviour consistent with the rest of the test remotes. Related to #3916

casperdcl added 6 commits June 12, 2020 12:57

gdrive: add open

0c4e859

Fixes iterative#3408 Related iterative#2865 Fixes iterative#3897

dependency: add gdrive

78dc1b9

test: api: open: gdrive

3cfdcc3

rollback pydrive dependency

2a140c9

Revert "dependency: add gdrive"

405684d

This reverts commit fd33326.

tests: remotes: GDrive: fix get_url

802577f

casperdcl added 2 commits June 12, 2020 12:59

tests: api: fully test GDrive

61c1125

minor typo fix

a6f93c0

casperdcl force-pushed the gdrive-stream branch from dc33fde to a6f93c0 Compare June 12, 2020 12:00

attempt gdrive tests fix

7285161

efiop reviewed Jun 13, 2020

View reviewed changes

tests/func/test_api.py Outdated Show resolved Hide resolved

casperdcl added 4 commits June 13, 2020 14:19

fix gdrive credentials config

5aee39a

replace GDrivePathNotFound => FileMissingError

ba52fc1

fix test_open_external[GDrive]

a84311a

add ensure_dir_scm

8125a65

casperdcl commented Jun 13, 2020

View reviewed changes

casperdcl marked this pull request as ready for review June 13, 2020 18:33

efiop reviewed Jun 13, 2020

View reviewed changes

dvc/exceptions.py Outdated Show resolved Hide resolved

minor exception fix

8ae3016

efiop merged commit 4d140a9 into iterative:master Jun 13, 2020

casperdcl mentioned this pull request Jun 13, 2020

ref: GDrive supported by open() iterative/dvc.org#1435

Closed

efiop mentioned this pull request Jun 15, 2020

"AttributeError: NoneType object has no attribute 'wdir'" on dvc get-url call #3897

Closed


		remote_params = [S3, GCP, Azure, OSS, SSH, HDFS]
		remote_params = [S3, GCP, Azure, GDrive, OSS, SSH, HDFS]

gdrive: add open #3916

gdrive: add open #3916

Uh oh!

Conversation

casperdcl commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

casperdcl commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop commented Jun 4, 2020

Uh oh!

Uh oh!

casperdcl Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

efiop commented Jun 13, 2020

Uh oh!

efiop commented Jun 13, 2020

Uh oh!

Uh oh!

casperdcl commented May 30, 2020 •

edited

Loading

casperdcl commented May 31, 2020 •

edited

Loading

codecov bot commented May 31, 2020 •

edited

Loading

efiop Jun 13, 2020 •

edited

Loading

casperdcl Jun 13, 2020 •

edited

Loading