Skip to content

Apply the bit-vs-byte suffix rule to the binary size units - #8539

Open
vineethsaivs wants to merge 1 commit into
huggingface:mainfrom
vineethsaivs:fix/binary-unit-bit-sizes
Open

Apply the bit-vs-byte suffix rule to the binary size units#8539
vineethsaivs wants to merge 1 commit into
huggingface:mainfrom
vineethsaivs:fix/binary-unit-bit-sizes

Conversation

@vineethsaivs

Copy link
Copy Markdown
Contributor

Symptom

convert_file_size_to_int() treats a lowercase trailing b as bits and divides by 8, but only for the decimal units. The binary units ignore the suffix entirely:

>>> from datasets.utils.py_utils import convert_file_size_to_int
>>> convert_file_size_to_int("1Gb")    # gigabit, correct
125000000
>>> convert_file_size_to_int("1Gib")   # gibibit, should be 2**30 // 8
1073741824
>>> convert_file_size_to_int("1GiB")   # gibibyte
1073741824

"1Gib" and "1GiB" return the same number, so a size given in gibibits is read as gibibytes and comes out 8x too large. The same holds for Kib, Mib, Tib and Pib.

Root cause

Ten sibling branches in the same function, and only five of them apply the rule:

if size.upper().endswith("GIB"):
    return int(size[:-3]) * (2**30)          # no bit handling
...
if size.upper().endswith("GB"):
    int_size = int(size[:-2]) * (10**9)
    return int_size // 8 if size.endswith("b") else int_size

This is not a judgement call about whether bit units should be supported: #4205 ("Fix convert_file_size_to_int for kilobits and megabits") established that they are, and gave the exact two-line form above. It only touched KB/MB/GB. #5171 later added PB/TB and copied the decimal shape, so those are right too. The *iB branches sit above them and were never updated.

Fix

Apply the same two lines to the five binary branches, so all ten units agree on what a trailing b means. No other behaviour changes: an uppercase B takes the identical path it took before.

Why it matters

The parsed value feeds max_shard_size, in Dataset.save_to_disk, Dataset.push_to_hub, IterableDataset.push_to_hub, DatasetBuilder.download_and_prepare and the Spark builder, plus MAX_ROW_GROUP_SIZE in arrow_writer. An 8x overshoot there is silent: it produces oversized shards rather than an error.

Test

tests/test_py_utils.py had no coverage for this function. Added a parametrized test_convert_file_size_to_int over all ten units in both spellings (plus an int passthrough), and test_convert_file_size_to_int_invalid_unit.

# against this branch
$ python -m pytest tests/test_py_utils.py -k convert_file_size -q
22 passed, 74 deselected

# same tests against main
$ python -m pytest tests/test_py_utils.py -k convert_file_size -q
5 failed, 17 passed, 74 deselected
FAILED test_convert_file_size_to_int[1Kib-128]
FAILED test_convert_file_size_to_int[1Mib-131072]
FAILED test_convert_file_size_to_int[1Gib-134217728]
FAILED test_convert_file_size_to_int[1Tib-137438953472]
FAILED test_convert_file_size_to_int[1Pib-140737488355328]

# whole file
$ python -m pytest tests/test_py_utils.py -q
95 passed, 1 skipped

ruff check and ruff format --check are clean on both files.

convert_file_size_to_int() reads a lowercase trailing "b" as bits and divides by
8, but only on the decimal units. The five binary branches (KiB, MiB, GiB, TiB,
PiB) return early with the plain multiplier, so "1Gib" is read as one gibibyte
instead of one gibibit and comes out 8x too large.

The rule itself was established for KB/MB/GB in huggingface#4205 and the *iB branches were
simply never updated; PB/TB in huggingface#5171 later copied the decimal shape and got it
right. Apply the same two-line form to the binary units so all ten agree.

The value feeds max_shard_size in Dataset.save_to_disk, push_to_hub,
IterableDataset.push_to_hub and the builder, so an oversized parse means
oversized shards rather than an error.

Tests cover all ten units in both spellings plus an int passthrough and an
unknown unit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant