Stream progress

[casperdcl]

• update to tqdm>=4.40.0
• tidy remote.gs to use Tqdm.wrapattr
  • upload
  • download
• modularise remote.gdrive
• tidy remote.gdrive to use Tqdm.wrapattr
  • upload
  • download (no-progress message for now; to fix in Google Drive support further enhancements #2865 (comment))
• continues GS progress for push & pull #2809
• continues GDrive remote support #2551
• depends on next release v4.40.0 tqdm/tqdm#852 <- add wrapattr tqdm/tqdm#844 <- Add support for data streams (with known total size) tqdm/tqdm#84
• fixes tidy stream progress #2829

[casperdcl]

I don't understand the purpose of the isolated self.drive lines on

dvc/dvc/remote/gdrive.py

Lines 150 to 156 in 75f8b9c

	self.drive
	return self._cached_dirs
	@property
	def cached_ids(self):
	if not hasattr(self, "_cached_ids"):
	self.drive

nor can I see where that variable is defined. Trying to debug what to do with

dvc/dvc/remote/gdrive.py

Lines 119 to 125 in 75f8b9c

	gdrive_file = self.drive.CreateFile({"id": file_id})
	with Tqdm(
	desc=progress_name,
	total=int(gdrive_file["fileSize"]),
	disable=no_progress_bar,
	):
	gdrive_file.GetContentFile(to_file)

(i.e. does GetContentFile support a file object rather than string?)

[casperdcl]

I assume source here? https://github.com/gsuitedevs/PyDrive/blob/68cea204cdcd10bab9321580164c8f4961385a0f/pydrive/files.py#L202 - in which this is non-trivial (by far easiest to open a PR upstream) - let me know if you want me to.

[efiop]

@casperdcl PyDrive guys are not very responsive, so I would create a PR and create GetContentFileObj in our code to use for now.

[casperdcl]

GetContentFile is not easy to patch (there's a stream.getvalue() used at some point which means the file is downloaded in-memory before being written to the progress-wrapped file) will leave this issue for #2865 (comment)

[casperdcl]

maybe we should remove this completely for now... Right now, a user will see something like "0/10GB" for 10mins before suddenly the operation completes and the bar disappears. It's probably better if instead we say "Downloading...".

[efiop]

@casperdcl Let's keep it as is, at least there are some notifications (binary, but still) 🙂

[casperdcl]

yes but if I saw a "0/10GB" bar doing nothing for a minute I'd assume something was wrong and nothing was happening & kill the process

[efiop]

@casperdcl But what are our options at this point? I suppose:

1. remove it (which will just leave an empty screen, which is worse)
2. add some kind of temporary spinner to keep the user entertained
3. dive into pydrive and hack a proper solution for us and submit it to pydrive

am I missing something? If not, which one do you suggest? 🙂

[casperdcl]

Definitely option 2 (well, not a spinner, but just the word "Downloading...")

1. remove it: agreed bad
2. change to something else: technically correct
3. properly fix: would take a while and better to do in other PR(s)
4. leave as-is (0/10GB hanging): I think worse than (1) because I'd be more likely to incorrectly think the program was hanging

[efiop]

@casperdcl The word will be static? If so, it doesn't seem very different from the current approach. Maybe Downloading... (no progress available) to explain? 🙂 Though, we've just merged gdrive support and haven't even documented it yet, so perhaps we are overthinking the importance of this at this particular moment and we probably could simply wait for that stuff to get properly fixed before publically announcing the support.

[shcheklein]

@shcheklein my 2cs - I would vote for spinner in these cases. And I think it's good to have them in our toolkit anyways - will give us more flexibility in terms of building UI. @casperdcl do you have a spinner/super-compact mode for TQDM ? :)

[casperdcl]

yeah idk I still prefer a correct static word to a misleading and incorrect 0/10GB. Of course we could try to improve the static word but atm tqdm doesn't support threaded spinners (i.e. a background thread updates the screen even though no calls to update or __iter__ were performed)

[casperdcl]

Always happy to accept a PR :)

[efiop]

Guys, we are overthinking this one, it will probably get fixed properly very soon, so any placeholder will do (even the previous one, we used to have it for gs for the longest time and didn't receive that much complains, so I'm not really worried about the current temp solution). I'm fine with the current static "Downloading" for now.