Remove num_retries parameter from load_table_from_*() methods

[plamut]

When dealing with missing timeout for resumable upload requests, I noticed several comments that the num_retries parameter is deprecated and is planned to be removed. If this is still the case, we can use the opportunity and remove it in v3.

Examples:

• In _do_resumable_upload()
• In _initiate_resumable_upload()
• In _do_multipart_upload()

[plamut]

@tswast Those comments were added at least two years ago, and we haven't actually been issuing any deprecation warnings.

Are the comments still valid?

[tswast]

Looks like they were there even 4 years ago. googleapis/google-cloud-python#4136

Edit: even before that: googleapis/google-cloud-python#3555

Edit: I think we actually inherited it from GCS: googleapis/google-cloud-python#3378

Edit: This was the PR with original deprecation/removal: googleapis/google-cloud-python#3362

[tswast]

I imagine the intent was to deprecate it in favor of a retry object? Doesn't look like we actually did that, though.

Maybe in v3 we add the retry logic -- potentially similar to job_retry in query, seeing as this involves starting the stream from the beginning, which is slightly different from the per-request retry object?

If we get that working, then I think in v3 we just start warning for num_retries rather than remove it, seeing as we haven't actually had a replacement available.

[tswast]

Looks like the intent was to replace it with a https://googleapis.dev/python/google-resumable-media/latest/resumable_media/common.html?highlight=retrystrategy#google.resumable_media.common.RetryStrategy, actually?

[tswast]

Looks like google-cloud-storage has a converter from Retry -> RetryStrategy.

https://github.com/googleapis/python-storage/blob/af9c9dc83d8582167b74105167af17c9809455de/google/cloud/storage/blob.py#L1912

We might want to do something similar.

Edit: See where this was added in googleapis/python-storage#447

[plamut]

I imagine the intent was to deprecate it in favor of a retry object? Doesn't look like we actually did that, though.

That's quite possible. A retry strategy is much more expressive than a plain timeout value.

I agree that issuing deprecation warnings first would be a more user-friendly approach. I wouldn't necessarily include this in the 3.0 release, though, maybe just the deprecation warning? IMHO there's more value in finalizing the first v3 release by the end of the year, and only include this feature in 3.0.0 if the capacity allows for it, i.e. if it doesn't cause delays.

[tswast]

Agreed that it's a low priority and that we'd probably only include the deprecation warning + retry feature in v3. A stable v3 is definitely more important, and this could happen post-v3 release.

[chalmerlowe]

Gonna mark this as a P3 priority based on the above conversation.

[chalmerlowe]

This is still on the back burner. If we intend to do this, the first steps are:

• identify where the num_retries parameter is still being used AND needs to be removed
• add deprecation warnings to the code relative to those uses

[Linchin]

I wonder if we still need to remove num_retries, as it is passed as a parameter max_retries into google resumable media, which doesn't seem to be deprecated any time soon. We might unify and clarify the multiple levels of retries we use in the load functions, but that would be its own issue.

@tswast @chalmerlowe
Please let me know what you think about this :)

[tswast]

@chalmerlowe , @Linchin is correct. There are at least three types of retries that are relevant to load jobs:

1. Individual API request retries. Our client uses the naming convention retry for these.
2. Resumable media retries. There is a multi-api request protocol for uploading files to Google, shared by Drive, BigQuery, and Cloud Storage. Errors can occur mid-upload, in which case we have to seek back to the start of the file and try the upload again. This is currently managed by the num_retries parameter. Which is oddly named because it predates the retries in (1)
3. Job level retries. These get a new job ID and try the whole job again if the job failed for a retryable reason (such as rate limiting). This is currently only implemented for query jobs via the job_retry parameter. It would be very beneficial to do this for load jobs also.

I would be in favor of deprecating num_retries in favor of a new name that reflects the purpose of being specific to resumable media, but not removing it.

I do recommend reaching out to the GCS team to see if they have plans for improving retry support in resumable media. I recall there was a project along those lines in 2020.

[tswast]

#1071 (comment)

Just re-read the whole thread and this comment appears to be the most relevant. I don't know if it makes sense to add a new parameter or reuse the same retry object for two different purposes. Would be good to sync with GCS in that regard.