Set corpora and driveId params values based on type of remote disk - shared or user's. #3239

maxhora · 2020-01-26T12:54:06Z

PR is not yet ready: will require update of PyDrive2 along with this PR.

iterative/PyDrive2#6 should be merged first.

shcheklein · 2020-01-26T17:55:20Z

dvc/remote/gdrive.py

@@ -109,8 +109,11 @@ def init_drive(self):
    def gdrive_upload_file(
        self, args, no_progress_bar=True, from_file="", progress_name=""
    ):
+        param = {"supportsAllDrives": True}


I think we can keep support*Drivers family of params inside? They are deprecated anyway and will be enabled by default soon? Also were they present even before we forked the PyDrive?

In original PyDrive from pip some support of related params was added ( deprecated supportsTeamDrives was used ), but some bugs were fixed in PyDrive's master and not released to pip.

I think we can keep support*Drivers family of params inside?

Sounds good.

dvc/remote/gdrive.py

maxhora · 2020-01-27T23:14:05Z

@shcheklein

support*Drivers params are moved back into PyDrive2 (related Update deprecated params to support Teamdrives PyDrive2#6 should be merged )
Shared driveId is obtained from the remote root directory properties (turned out that GDrive provides property driveId for items at shared drives only).
If driveId is not empty corpora is set to drive otherwise default (should be analog to user in GDrive API v3). Since allDrives corpora is not used, hopefully, incompleteSearch will not occur.

shcheklein · 2020-01-27T23:16:43Z

@Maxris do we still need to update PyDrive to remove corpora/corpus from it?

also, just to double check - I hope that all these old support*Drivers (there are two sets of those btw, and we use older versions) do not contradict with corpora.

shcheklein · 2020-01-27T23:17:14Z

@Maxris tests are failing on Travis

maxhora · 2020-01-27T23:18:32Z

@shcheklein yes, iterative/PyDrive2#6 should be merged into PyDrive2. Then we need to release new version of PyDrive2 and update this PR to pin newer version to ensure that Travis tests are passing.

maxhora · 2020-01-27T23:20:54Z

@Maxris do we still need to update PyDrive to remove corpora/corpus from it?

also, just to double check - I hope that all these old support*Drivers (there are two sets of those btw, and we use older versions) do not contradict with corpora.

right, iterative/PyDrive2#6 does exactly above - removes corpus and updates older version of params to newer.

dvc/remote/gdrive.py

shcheklein · 2020-01-27T23:28:05Z

@Maxris

right, iterative/PyDrive2#6 does exactly above - removes corpus and updates older version of params to newer.

good, I've merged it. We will need to release and update the version in the setup.py and conda.

shcheklein · 2020-01-27T23:32:11Z

@Maxris 1.4.2 is released, let's update deps please (and don't forget to update conda when we ready to merge)

shcheklein · 2020-01-27T23:34:25Z

dvc/remote/gdrive.py

@@ -104,13 +104,16 @@ def init_drive(self):
                ),
            )
        )
+        self.corpora = "allDrives"
+        self.remote_drive_id = ""


would None do here?

shcheklein · 2020-01-27T23:35:08Z

dvc/remote/gdrive.py

@@ -104,13 +104,16 @@ def init_drive(self):
                ),
            )
        )
+        self.corpora = "allDrives"


do we need this default value? if not it would be safer to keep them None, to make sure that we don't hit allDrive after some changes in the future.

yep, we need allDrives initially, since we don't know initially where the root directory is located - at shared or user's drive.

do we hit any APIs that require corpora though before we able to figure out this?

well, it looks like it might occur. I will look into possibility to put the assert into PyDrive2 to catch this.

not into PyDrive, probably 🤔 ... I meant that I still don't quite understand why do we need this default ... it's very close to the beginning where we get the actual value for it - default or drive, right? so why do we need this default? is it needed for the period we are still not sure is a shared drive or not? do we hit any APIs that require this value?

@shcheklein sorry, I meant, that, yes, currently ListFile API is called. But it is possible to avoid this, will update the code.

gotcha, yep, let's avoid this if possible and remove the default value

shcheklein · 2020-01-27T23:59:32Z

dvc/remote/gdrive.py

+    @gdrive_retry
+    def get_remote_drive_id(self, remote_id):
+        param = {"id": remote_id}
+        # drive.CreateFile creates only local GoogleDriveFile object


...
# it does not create a file on the remote

codecov · 2020-01-28T00:15:39Z

Codecov Report

Merging #3239 into master will decrease coverage by 0.08%.
The diff coverage is 65.21%.

@@            Coverage Diff             @@
##           master    #3239      +/-   ##
==========================================
- Coverage   92.66%   92.58%   -0.09%     
==========================================
  Files         139      139              
  Lines        8744     8764      +20     
==========================================
+ Hits         8103     8114      +11     
- Misses        641      650       +9

Impacted Files	Coverage Δ
dvc/remote/gdrive.py	`89.84% <65.21%> (-3.38%)`	⬇️
dvc/updater.py	`54.65% <0%> (-1.17%)`	⬇️

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 242b5b1...7b0e44e. Read the comment docs.

maxhora · 2020-01-28T01:15:25Z

@shcheklein usage of allDrives is removed, but it seems I have found the pull issues if the URL consists only from single folder ID, like gdrive://18HB_cxbipUlERdQK-HNT8EbGiopAMjjY
Going to check why it happens

shcheklein

Looks good to me! Let's update conda when it's merged.

efiop · 2020-01-28T22:42:20Z

@Maxris Is this still WIP? 🙂

maxhora self-assigned this Jan 26, 2020

maxhora requested review from efiop and shcheklein January 26, 2020 13:29

shcheklein reviewed Jan 26, 2020

View reviewed changes

dvc/remote/gdrive.py Show resolved Hide resolved

maxhora changed the title ~~[WIP] Use corpora param to list all drives~~ [WIP] Set corpora and driveId params values based on type of remote disk - shared or user's. Jan 27, 2020

shcheklein reviewed Jan 27, 2020

View reviewed changes

dvc/remote/gdrive.py Show resolved Hide resolved

shcheklein reviewed Jan 27, 2020

View reviewed changes

maxhora force-pushed the fix-shared-drives-support branch 2 times, most recently from f3a91e0 to 08bf8fd Compare January 27, 2020 23:54

shcheklein reviewed Jan 27, 2020

View reviewed changes

gdrive: support corpora and driveId

7b0e44e

maxhora force-pushed the fix-shared-drives-support branch from 17397b1 to 7b0e44e Compare January 28, 2020 19:50

shcheklein approved these changes Jan 28, 2020

View reviewed changes

maxhora changed the title ~~[WIP] Set corpora and driveId params values based on type of remote disk - shared or user's.~~ Set corpora and driveId params values based on type of remote disk - shared or user's. Jan 28, 2020

efiop merged commit d16e71d into master Jan 28, 2020

delete-merged-branch bot deleted the fix-shared-drives-support branch January 28, 2020 22:44

weekly-digest bot mentioned this pull request Feb 2, 2020

Weekly Digest (26 January, 2020 - 2 February, 2020) #3272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set corpora and driveId params values based on type of remote disk - shared or user's. #3239

Set corpora and driveId params values based on type of remote disk - shared or user's. #3239

maxhora commented Jan 26, 2020 •

edited

Loading

shcheklein Jan 26, 2020

maxhora Jan 26, 2020 •

edited

Loading

maxhora commented Jan 27, 2020 •

edited

Loading

shcheklein commented Jan 27, 2020

shcheklein commented Jan 27, 2020

maxhora commented Jan 27, 2020

maxhora commented Jan 27, 2020

shcheklein commented Jan 27, 2020

shcheklein commented Jan 27, 2020

shcheklein Jan 27, 2020

shcheklein Jan 27, 2020

maxhora Jan 27, 2020 •

edited

Loading

shcheklein Jan 27, 2020

maxhora Jan 27, 2020

shcheklein Jan 27, 2020

maxhora Jan 27, 2020

shcheklein Jan 28, 2020

shcheklein Jan 27, 2020

codecov bot commented Jan 28, 2020 •

edited

Loading

maxhora commented Jan 28, 2020

shcheklein left a comment

efiop commented Jan 28, 2020

Set corpora and driveId params values based on type of remote disk - shared or user's. #3239

Set corpora and driveId params values based on type of remote disk - shared or user's. #3239

Conversation

maxhora commented Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

maxhora Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

maxhora commented Jan 27, 2020 • edited Loading

shcheklein commented Jan 27, 2020

shcheklein commented Jan 27, 2020

maxhora commented Jan 27, 2020

maxhora commented Jan 27, 2020

shcheklein commented Jan 27, 2020

shcheklein commented Jan 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxhora Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2020 • edited Loading

Codecov Report

maxhora commented Jan 28, 2020

shcheklein left a comment

Choose a reason for hiding this comment

efiop commented Jan 28, 2020

maxhora commented Jan 26, 2020 •

edited

Loading

maxhora Jan 26, 2020 •

edited

Loading

maxhora commented Jan 27, 2020 •

edited

Loading

maxhora Jan 27, 2020 •

edited

Loading

codecov bot commented Jan 28, 2020 •

edited

Loading