Repository navigation
Conversation
A name such as f32e8m0 was parsed as float32 because 0 was treated as a missing width. The same substitution turned f16e0m10 into float16. Signed-off-by: Sasha Mitchell <sash.t.mitchell@gmail.com>
 [Low impact] The change is limited to dtype parsing in DALI's experimental dynamic API. The PR should not merge until the existing Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@SashaMIT please address comments from automatic review |
DType.__init__ records the temporary type before the registered-type lookup. A failed parse of f32e8m0 left that name in the cache, so the next parse returned it. The error now names the input. Signed-off-by: Sasha Mitchell <sash.t.mitchell@gmail.com>
|
The rejected name is no longer cached, and the error names the type. |
| if _name2type.get(t.name) is t: | ||
| del _name2type[t.name] |
There was a problem hiding this comment.
bfloat16
[Bug] A failed dtype("f16e8m7") now breaks later dtype("bfloat16") calls. The temporary DType gets the name bfloat16 and overwrites its existing entry before this check runs. Because bfloat16 has no type_id, the lookup fails and this cleanup deletes its name. Later calls raise ValueError instead of returning ndd.bfloat16.
Save and restore any prior entry, or avoid caching temporary types until the lookup succeeds.
| raise ValueError( | ||
| f"{name} is not a registered type. Expected a name such as f16 or f32." | ||
| ) from None |
There was a problem hiding this comment.
The proprosed error message may be misleading.
| raise ValueError( | |
| f"{name} is not a registered type. Expected a name such as f16 or f32." | |
| ) from None | |
| raise ValueError(f"{name} is not a registered type.") from None |
| self.exponent_bits = exponent_bits or DType._default_exponent_bits(bits, True) | ||
| self.significand_bits = significand_bits or DType._default_significand_bits(bits, True) | ||
| if exponent_bits is None: | ||
| exponent_bits = DType._default_exponent_bits(bits, True) |
There was a problem hiding this comment.
| exponent_bits = DType._default_exponent_bits(bits, True) | |
| exponent_bits = DType._default_exponent_bits(bits, True) | |
| elif exponent_bits < 1: | |
| raise ValueError(f"Invalid number of exponent bits: {exponent_bits}") |
| if exponent_bits is None: | ||
| exponent_bits = DType._default_exponent_bits(bits, True) | ||
| if significand_bits is None: | ||
| significand_bits = DType._default_significand_bits(bits, True) |
There was a problem hiding this comment.
| significand_bits = DType._default_significand_bits(bits, True) | |
| significand_bits = DType._default_significand_bits(bits, True) | |
| elif significand_bits < 1: | |
| raise ValueError(f"Invalid number of significand bits: {significand_bits}") |
A written 0 is an invalid exponent or significand, not a missing width. An unregistered name no longer suggests f16 or f32.
|
A width below 1 is rejected in 4ebf21f. f32e8m0 reports an invalid significand, f16e0m10 reports an invalid exponent, and an unregistered name no longer suggests f16 or f32. |
| if exponent_bits is None: | ||
| exponent_bits = DType._default_exponent_bits(bits, True) | ||
| elif exponent_bits < 1: | ||
| raise ValueError(f"Invalid number of exponent bits: {exponent_bits}") |
There was a problem hiding this comment.
[Style] The new errors for exponent_bits and significand_bits show the rejected value but do not tell users that the minimum is 1. They also lack final periods. The repository requires errors to name the parameter, show expected versus actual values, and end as complete sentences.
Use messages such as Invalid exponent_bits: got {exponent_bits}, expected at least 1. for both checks, and update the test expectations.
Rule Used: Error messages must name the affected parameter/operator, show expected vs. actual values, and end as a complete sentence. Not "is 5" but "got 5 channels, expected 1 or 3.". Use make_string(...) for C++ concatenation; f-strings in Python. (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Category:
Bug fix (non-breaking change which fixes an issue)
Description:
DType.parse("f32e8m0")came back as float32.DType.parse("f16e0m10")came back as float16.The float constructor used
width or default. A written 0 is falsy, so it was replaced with the IEEE default (23 significand bits, or 5 exponent bits). Those names are not registered DALI types. The parser now raises ValueError, the same as any other unknown name. Omitted widths still take the IEEE default, sof32andf16are unchanged.Additional information:
Affected modules and functionalities:
nvidia.dali.experimental.dynamic.DType.parseanddtype().Key points relevant for the review:
Only a missing width (
None) takes the default. A written 0 stays 0, then fails the registered-type lookup.Tests:
dali/test/python/experimental_mode/test_type.py:test_explicit_zero_float_field_is_not_replaced_with_the_defaultOn main the test fails because
f32e8m0is accepted as float32. With this change it passes.Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A