updated default block size #509

anjaliratnam-msft · 2025-08-11T19:12:48Z

Updated the default block size to be 50 MiB so AzureBlobFileSystem and AzureBlobFile are more consistent. This also addresses the github issue where the chunk_size of 1GB results in a Timeout Error and also respects the block_size parameter sent in when opening a file.

I also created this gist to show that the performance is not impacted negatively when increasing the default from 5 to 50 MiB and performance improves when reducing the chunk size from 1 GB to 50 MiB.

kyleknap · 2025-08-12T18:17:02Z

@martindurant just giving a heads up... this PR is still a work-in-progress and we'll let you know when it is ready for a full review, but wanted to get your initial thoughts on the direction.

This PR is a result of this conversation that we had here: #508 (comment), and we see as a precursor to this PR: #508 since any from perf gains from concurrency introduced will likely be degraded by the fact that 1GiB payload blocks are being used for concurrent PUTs.

Before we get to far along in this PR, we did want to check with you to make sure you are generally onboard with the changes the PR will be eventually introducing:

Always use the configured blocks size for individual parts uploaded. Specifically, in this case if a large write() occurs (e.g., larger than 5 MB), adlfs would always use 1GiB, which can cause network problems such as @anjaliratnam-msft and the referenced GitHub issue: Data-Upload: Timeout problems and chunk size #494 was reporting.
Update the default block size to be 50 MiB everywhere. Right now the default chunk size is inconsistently set (e.g. it's 4 MiB in the FileSystem and 5 MiB in the file object. This will make it consistent with s3fs: Concurrency in pipe() s3fs#901 and overall we believe this will speed up adlfs (we are still doing testing around this but and will update that gist).
Pass the blocksize from the AzureBlobFileSystem to the file object when fs.open() is called and no block_size override is set. This will make it more consistent with s3fs and ensure if you set a blocksize when instantiating the AzureBlobFileSystem it will be used for all methods and you don't have to reset the block_size again as part of the call to fs.open().

The main downsides (but still seems worth the tradeoff) that I could potentially see are:

Possible bump memory usage, especially when it comes to caching as part of reads
Shrinks the default max blob size that can be written using f.write() in some cases. Specifically, if someone was writing 1 GiB at a time, this will now be broken down into 50 MiB blocks and given that the max number of blocks is 50,000, the theoretical default max blob size is now 2.2 TiB (which is still really large). On the other end by increasing the block size to 50 MiB, it can increase the total blob size if you are doing small (MiB in size) writes at a time since more data will now fit into each block. So this point seems more like a wash.

Furthermore with change 3, it will allow users to more easily set the block size in the case the new defaults do not work for their use case.

Thoughts? Happy to elaborate on any of these more.

kyleknap

Looks good! I really like that we are collecting the perf numbers as well to make sure this change makes sense. Most of my feedback was on structuring the tests. The implementation itself looks solid.

kyleknap · 2025-08-12T20:58:19Z

adlfs/spec.py

@@ -1879,6 +1878,8 @@ def _open(
            is versioning aware and blob versioning is enabled on the releveant container.
        """
        logger.debug(f"_open:  {path}")
+        if block_size is None:


We should make sure to update the block_size docstring for this _open() method to:

Include "uploads" in the wording. Look like it only refers downloads

Update the wording to say that the parameter is an override and when not provided defaults to the blocksize on the file system.

kyleknap · 2025-08-12T21:00:33Z

adlfs/spec.py

@@ -2146,7 +2147,7 @@ async def _async_initiate_upload(self, **kwargs):

    _initiate_upload = sync_wrapper(_async_initiate_upload)

-    def _get_chunks(self, data, chunk_size=1024**3):  # Keeping the chunk size as 1 GB
+    def _get_chunks(self, data, chunk_size):


Looking through this more, it probably makes sense to just remove the chunk_size parameter from this helper method all together and just switch the sole reference to chunk_size with the instance property self.blocksize. Mainly, we are not really overriding this value now and the block size is already set in the constructor. So it makes it simpler.

kyleknap · 2025-08-12T21:09:44Z

adlfs/spec.py

@@ -69,7 +69,7 @@
    "is_current_version",
 ]
 _ROOT_PATH = "/"
-_DEFAULT_BLOCK_SIZE = 4 * 1024 * 1024
+_DEFAULT_BLOCK_SIZE = 50 * 2**20


We should also make sure to update the changelog in this PR. I'm thinking we can just use the three bullet points from this comment I made: #509 (comment) and make the wording a bit more succinct.

kyleknap · 2025-08-12T21:15:43Z

adlfs/tests/test_spec.py

+            assert actual_blocks == expected_blocks
+
+
+def test_block_size(storage):


It might be worth parameterizing this one to test out the different permutations of setting/omitting block sizes. To do this, we parameterize off of the input block size for both the file system and the fs.open() call and include the expected block size for both the file system and file object. We'd then add cases such as (feel free to adjust this or add more):

Assert defaults when block size is not set for either the file system or file object

Assert file system block size propagates to the file-like object

Assert that we can override the block_size for the fs.open() call

kyleknap · 2025-08-12T21:17:12Z

adlfs/tests/test_spec.py

+    fs = AzureBlobFileSystem(
+        account_name=storage.account_name,
+        connection_string=CONN_STR,
+        blocksize=5 * 2**20,


When implementing cases where we override the block size. It may be worth using values that are unlikely/odd defaults (e.g. 7 * 2 ** 20) to make it more clear this is an override value as opposed to a possible default.

kyleknap · 2025-08-12T21:24:35Z

adlfs/tests/test_spec.py

+    )
+
+    content = b"1" * (blocksize * 2 + 1)
+    with fs.open("data/root/a/file.txt", "wb", blocksize=blocksize) as f:


Let's remove blocksize from this call here as I'm not sure if this will affect anything since the argument used is block_size and it should suffice to set it in the file system.

kyleknap · 2025-08-12T21:30:56Z

adlfs/tests/test_spec.py

+        mocker.patch(
+            "azure.storage.blob.aio.BlobClient.commit_block_list", autospec=True
+        )
+        with patch(
+            "azure.storage.blob.aio.BlobClient.stage_block", autospec=True
+        ) as mock_stage_block:


For these patch statements, is there a particular reason why:

mocker.patch is not being used for both?

We are not following the pattern from other test cases where we import the BlobClient first e.g., here and patching it directly?

kyleknap · 2025-08-12T21:32:38Z

adlfs/tests/test_spec.py

+            f.write(content)
+            expected_blocks = math.ceil(len(content) / blocksize)
+            actual_blocks = mock_stage_block.call_count
+            assert actual_blocks == expected_blocks


Since we are patching the commit block list call as well, it may be worth to also assert the number of blocks in that call also match the expected number of blocks.

kyleknap · 2025-08-12T21:39:02Z

adlfs/tests/test_spec.py

@@ -2045,3 +2047,37 @@ def test_open_file_x(storage: azure.storage.blob.BlobServiceClient, tmpdir):
        with fs.open("data/afile", "xb") as f:
            pass
    assert fs.cat_file("data/afile") == b"data"
+
+
+@pytest.mark.parametrize("blocksize", [5 * 2**20, 50 * 2**20, 100 * 2**20])


I think we should be fine just making this a non-parameterized test. Not sure if there is much value in setting different block sizes. I'd say we just set the blocksize to somewhere in single MiB's and write() data that requires several blocks and maybe also make sure the last block does not completely fit completely into a block (e.g., is less than blocksize) to make sure the logic handles sizes that do not fit on block boundaries.

This will hopefully help simplify the scaffolding for the case and also take less time to run.

kyleknap · 2025-08-12T21:39:38Z

adlfs/tests/test_spec.py

+        ) as mock_stage_block:
+            f.write(content)
+            expected_blocks = math.ceil(len(content) / blocksize)
+            actual_blocks = mock_stage_block.call_count


It would also probably be worth asserting the actual data sizes used in each stage_block call.

kyleknap

Looks good! Just had some really small comments on the test and should be set from a code change perspective.

kyleknap · 2025-08-14T19:55:58Z

CHANGELOG.md

@@ -7,6 +7,9 @@ Unreleased
 - Fix issue where ``AzureBlobFile`` did not respect ``location_mode`` parameter
  from parent ``AzureBlobFileSystem`` when using SAS credentials and connecting to
  new SDK clients.
+- The block size is now used for uploads. Previously, it was always 1 GiB irrespective of the block size  


Let's slightly tweak this phrasing to:

- The block size is now used for partitioned uploads. Previously, 1 GiB was used for each uploaded block irrespective of the block size

Mainly technically was used previous in uploads but it was really only used for when the in-memory buffer was flushed.

kyleknap · 2025-08-14T19:59:04Z

CHANGELOG.md

@@ -7,6 +7,9 @@ Unreleased
 - Fix issue where ``AzureBlobFile`` did not respect ``location_mode`` parameter
  from parent ``AzureBlobFileSystem`` when using SAS credentials and connecting to
  new SDK clients.
+- The block size is now used for uploads. Previously, it was always 1 GiB irrespective of the block size  
+- Updated default block size to be 50 MiB 


Let's also add a sentence to this point to say how to revert back to the previous default. So something like:

Set `blocksize` for `AzureBlobFileSystem` or `block_size` when opening an `AzureBlobFile` to revert back to 5 MiB default.

Mainly, this will help anyone trying to understand how to go back to the previous default find it more easily than needing to dig through this PR.

kyleknap · 2025-08-14T20:15:54Z

adlfs/tests/test_spec.py

+    "filesystem_blocksize, file_blocksize, expected_blocksize",
+    [
+        (None, None, 50 * 2**20),
+        (50 * 2**20, None, 50 * 2**20),


For this case, it might make sense to make it a different value like that 7 * 2 ** 20 value to disambiguate from the default value.

kyleknap · 2025-08-14T20:16:50Z

adlfs/tests/test_spec.py

+        (None, None, 50 * 2**20),
+        (50 * 2**20, None, 50 * 2**20),
+        (None, 5 * 2**20, 5 * 2**20),
+        (50 * 2**20, 7 * 2**20, 7 * 2**20),


Same thing here, instead of using 50 * 2 ** 20 let's use a non-default value like 40 * 2 ** 20 to disambiguate from any defaults.

kyleknap · 2025-08-14T20:20:17Z

adlfs/tests/test_spec.py

+
+
+@pytest.mark.parametrize(
+    "filesystem_blocksize, file_blocksize, expected_blocksize",


It's also probably worth adding an expected_filesystem_blocksize to make the blocksize are expected at the filesystem level as well in this test case.

kyleknap · 2025-08-14T20:21:49Z

adlfs/tests/test_spec.py

+        block_size=file_blocksize,
+    )
+    assert f.blocksize == expected_blocksize
+    assert fs.blocksize == 50 * 2**20


We can probably remove this assertion assuming we add the expected_filesystem_blocksize parameter in the other parameterized test case.

kyleknap · 2025-08-14T20:28:59Z

adlfs/tests/test_spec.py

+    assert fs.blocksize == 50 * 2**20
+
+
+def test_override_blocksize(storage):


Ah so for this test case I was thinking, we'd:

Set the blocksize on the AzureBlobFileSystem

Assert that the AzureBlobFile without setting block_size does not inherit the the blocksize from 1. So after instantiation, we would just assert it is 50 * 2 ** 20, which should be its default.

Update the test name to better reflect that we are testing this code path does not inherit from the file system.

kyleknap · 2025-08-14T20:30:03Z

adlfs/tests/test_spec.py

@@ -2045,3 +2046,82 @@ def test_open_file_x(storage: azure.storage.blob.BlobServiceClient, tmpdir):
        with fs.open("data/afile", "xb") as f:
            pass
    assert fs.cat_file("data/afile") == b"data"
+
+
+def test_number_of_blocks(storage, mocker):


Let's update this test name to be test_uses_block_size_for_partitioned_uploads in order to be a bit more self-descriptive of what we are trying to test.

kyleknap

Looks good. Just had one more comment on the test. Otherwise, let's get the perf testing analysis in a spot that we are happy with and should be set.

kyleknap · 2025-08-15T16:33:01Z

adlfs/tests/test_spec.py

-        (50 * 2**20, None, 50 * 2**20),
-        (None, 5 * 2**20, 5 * 2**20),
-        (50 * 2**20, 7 * 2**20, 7 * 2**20),
+        (None, None, 50 * 2**20, None),


Instead of directly passing None, let's just treat it as omitting block_size/blocksize in the consturctor/open call. Mainly looking at the expected_filesystem_blocksize, I'm not sure that asserting that expected filesystem blocksize is None is what we want when we are mainly checking that it falls back to the default block size of 50 * 2 * 20.

kyleknap

@anjaliratnam-msft Looks good. I think we are in a good spot code wise so let's just wrap up the perf testing work and analysis to make sure we are confident in the changes.

@martindurant as a heads up we may be tweaking this plan here: #509 (comment). Specifically, for point 2, we are reconsidering whether we keep the default blocksize at 5 MiB instead of changing it to 50 MiB. We've been finding data that shows that when concurrency is introduced to writes, using 5 MiB blocks is significantly faster than 50 MiB blocks. We'll let you know when the perf results are in a good spot to review.

martindurant · 2025-08-18T16:39:28Z

We've been finding data that shows that when concurrency is introduced to writes, using 5 MiB blocks is significantly faster than 50 MiB blocks.

I'm sure it depends on how many blocks there are in total! So one 50MB block would be slower than 10x5MB, but if you have enough 50MB blocks to saturate too, I would expect it to be faster by reducing overhead.

kyleknap · 2025-08-18T17:35:48Z

@martindurant agreed it is definitely dependent on the number of blocks. One of the updates we are making to the perf testing is increasing the size of the blob being transferred to something fairly large (27 GiB) so that both the 5MiB and 50MiB block sizes reach concurrency saturation. Even at 5 GiB blob size, the 50 MiB block size was not necessarily saturating full concurrency so that may have been skewing result interpretation.

kyleknap

Looks great! Thank you for putting together these performance results. This is really helpful in making decisions on this.

I think it is awesome to see that with this PR alone, we will be reducing timeout/network errors by sending smaller upload payloads (50 MiB instead of possibly 1 GiB) and we will greatly be increasing the speed for small sequential writes (e.g. writing 1KB at a time scenario) as we will be reducing the number of PUTs being made.

Furthermore, it is great to see when we then we add on this PR that adds concurrency to partitioned uploads, it improves the speed of large writes (e.g., writing a large tensor in a PyTorch model) by 2x - 3x by default and can reach up to 5x improvement if you further tune blocksize and concurrency based on your workload. With both of these PRs, it really puts write performance on par with the read performance of adlfs now.

@martindurant this all should be ready to review. @anjaliratnam-msft includes the rationale in the perf results gist but we decided to stick with the approach of 50 MiB block size. We did continue to see when concurrency is enabled and saturated, 5 MiB block sizes squeezed out more speed than the 50 MiB sizes. However, the tradeoff in further limiting default blob sizes and performance degradation when concurrency is disabled outweighed these possible gains.

Let us know what you think!

martindurant · 2025-08-19T20:12:47Z

I am happy here.

updated default block size

23cd7f8

anjaliratnam-msft requested a review from kyleknap August 11, 2025 19:13

updates

2559f6e

kyleknap reviewed Aug 12, 2025

View reviewed changes

anjaliratnam-msft added 2 commits August 13, 2025 16:31

updates

1c8d123

updates

78e6ef5

kyleknap reviewed Aug 14, 2025

View reviewed changes

updates

b3c456b

kyleknap reviewed Aug 15, 2025

View reviewed changes

updates

7f28bbe

kyleknap reviewed Aug 15, 2025

View reviewed changes

kyleknap approved these changes Aug 19, 2025

View reviewed changes

martindurant merged commit b5b1c33 into fsspec:main Aug 19, 2025
8 checks passed

		assert actual_blocks == expected_blocks


		def test_block_size(storage):



		@pytest.mark.parametrize(
		"filesystem_blocksize, file_blocksize, expected_blocksize",

		assert fs.blocksize == 50 * 2**20


		def test_override_blocksize(storage):

updated default block size #509

updated default block size #509

Uh oh!

Conversation

anjaliratnam-msft commented Aug 11, 2025

Uh oh!

kyleknap commented Aug 12, 2025

Uh oh!

kyleknap left a comment

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!

kyleknap left a comment

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!

kyleknap left a comment

Choose a reason for hiding this comment

Uh oh!

kyleknap Aug 15, 2025 • 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!

kyleknap left a comment

Choose a reason for hiding this comment

Uh oh!

martindurant commented Aug 18, 2025

Uh oh!

kyleknap commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyleknap left a comment

Choose a reason for hiding this comment

Uh oh!

martindurant commented Aug 19, 2025

Uh oh!

Uh oh!

Uh oh!

kyleknap Aug 15, 2025 •

edited

Loading

kyleknap commented Aug 18, 2025 •

edited

Loading