Apply the bit-vs-byte suffix rule to the binary size units - #8539
Open
vineethsaivs wants to merge 1 commit into
Open
Apply the bit-vs-byte suffix rule to the binary size units#8539vineethsaivs wants to merge 1 commit into
vineethsaivs wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
convert_file_size_to_int()treats a lowercase trailingbas bits and divides by 8, but only for the decimal units. The binary units ignore the suffix entirely:"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 forKib,Mib,TibandPib.Root cause
Ten sibling branches in the same function, and only five of them apply the rule:
This is not a judgement call about whether bit units should be supported: #4205 ("Fix
convert_file_size_to_intfor kilobits and megabits") established that they are, and gave the exact two-line form above. It only touchedKB/MB/GB. #5171 later addedPB/TBand copied the decimal shape, so those are right too. The*iBbranches 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
bmeans. No other behaviour changes: an uppercaseBtakes the identical path it took before.Why it matters
The parsed value feeds
max_shard_size, inDataset.save_to_disk,Dataset.push_to_hub,IterableDataset.push_to_hub,DatasetBuilder.download_and_prepareand the Spark builder, plusMAX_ROW_GROUP_SIZEinarrow_writer. An 8x overshoot there is silent: it produces oversized shards rather than an error.Test
tests/test_py_utils.pyhad no coverage for this function. Added a parametrizedtest_convert_file_size_to_intover all ten units in both spellings (plus anintpassthrough), andtest_convert_file_size_to_int_invalid_unit.ruff checkandruff format --checkare clean on both files.