Possible issue with latest s3fs (2024.3.0): max_concurrency kwarg

[ryan-williams]

s3fs 2024.3.0 (released yesterday) added a max_concurrency kwarg (fsspec/s3fs#848), and today I have a job failing during a dvc pull from S3, referencing an unexpected keyword argument 'max_concurrency':

ERROR: failed to transfer '54a252d859eea2207da0fb933661dca0' - S3FileSystem._get_file() got an unexpected keyword argument 'max_concurrency'
ERROR: failed to pull data from the cloud - 1 files failed to download

(GHA link)

I was unable to repro it locally (with most of the same relevant versions: dvc{,_s3}, *boto*, s3fs), but pinning s3fs<=2024.2 allowed the same dvc pull to succeed (GHA link).

Mentioning here in case others run into it / can better triage.

[shcheklein]

Thanks @ryan-williams . Should be fixed by fsspec/s3fs#863 .

We should probably for now do a some workaround here also (in case of s3 avoid using max_concurrency for get file) in dvc-objects. Also, we should see why it was not implemented in the first place - @pmrowla if could give a context - that would be helpful - was it involved and / or nor needed?

[pmrowla]

Supporting concurrent chunked downloads is more complex than uploads, so it was not implemented due to time constraints.

For uploads (fsspec _put_file), the S3 API inherently supports uploading chunks out of order in multipart uploads. Re-assembling them in order is handled on the amazon/server end.

For downloads (fsspec _get_file), doing a chunked/concurrent download requires downloading the required byte-ranges and then re-assembling them locally. There is no built-in support for the out of order re-assembly step in the S3 API or in the botocore libraries. (For adlfs, the microsoft azure python sdk does include built-in support for concurrent chunked downloads, which is why it's supported in adlfs _get_file)

Since there is no guarantee that downloads will be completed in order, this means completed chunks need to either be kept in memory or written out to temp files on disk before re-assembling them at the end of the download operation (once all chunks are available, or at least when the "next" sequential chunk is available).

This is something that can be implemented in s3fs, but IMO it would be better for it to be implemented at the outer client level (i.e. the client calling fsspec, in this case dvc-objects) which would make chunked downloads supported for all filesystems that support downloading a specific byte range (which is essentially every fsspec implementation).

[martindurant]

Since there is no guarantee that downloads will be completed in order, this means completed chunks need to either be kept in memory or written out to temp files on disk before re-assembling them at the end of the download operation

I think all local filesystems support seeking beyond the end of the file to write data, so reassembly should not be hard. On linux this even produces "sparse" files (on windows you get padding), which is not important in this case because we intend to fill all the gaps.

[dberenbaum]

Should we create a separate issue for concurrent chunked downloads?

Edit: And can we then close this issue? I see @martindurant released 2024.03.01.

[martindurant]

Should we create a separate issue for concurrent chunked downloads?

It's worth making a note, but someone should benchmark that it actually makes a difference. Currently we go concurrent over all files (subject to a throttle for limiting number of file descriptors) but each file streams. Although windows does allow seeking beyond a file's end to extend it, I wonder if it allows multiple writers in a single file. If not, each task would need to open/seek/write/close every time.

[skshetry]

Closed by fsspec/s3fs#863, and released in s3fs==2024.3.1.