Replace asserts with ValueError in chat_template/conversation.py#94
Open
lonexreb wants to merge 1 commit intoNVlabs:mainfrom
Open
Replace asserts with ValueError in chat_template/conversation.py#94lonexreb wants to merge 1 commit intoNVlabs:mainfrom
lonexreb wants to merge 1 commit intoNVlabs:mainfrom
Conversation
Continues the pattern of NVlabs#50 (create_message), NVlabs#73 (load_physical_aiavdataset), and NVlabs#77 (assert→ValueError in public APIs): asserts can be stripped with `python -O` (or by callers using `PYTHONOPTIMIZE=1`), so any input-validation guard expressed as an assert silently disappears in optimized runs. Convert all four asserts in src/alpamayo_r1/chat_template/conversation.py to explicit `if ... raise ValueError(...)`: - get_component_str L38: callers must pass exactly one of `content_str` or `padding_str` (XOR). The new check `(a is None) == (b is None)` catches both "neither provided" and "both provided" with the same message. - construct_image L115: `camera_ids` must be sorted ascending; the unsorted case now raises with the offending tensor in the message for easier debugging instead of a bare AssertionError. - construct_cot L195: `data["cot"]` must exist when not asking the model to generate it. - construct_meta_action L227: `data["meta_action_strings"]` must exist when not asking the model to generate it. Pure validation hardening. Behavior with optimization off is identical; with optimization on, the checks now actually run. Signed-off-by: lonexreb <[email protected]>
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
1 participant
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.
Why
Continues the pattern of #50 (
create_message), #73 (load_physical_aiavdataset), and #77 (assert→ValueErrorin public APIs):src/alpamayo_r1/chat_template/conversation.pyhas four such asserts that all guard caller-provided inputs to public functions. They're invisible to anyone running optimized Python.What
Convert all four asserts to explicit
if … raise ValueError(...):get_component_strcontent_str/padding_str(XOR)construct_imagecamera_idsis sorted ascendingconstruct_cot"cot"key present indatawhen not asking the model to generate itconstruct_meta_action"meta_action_strings"key present indatawhen not asking the model to generate itTwo readability wins beyond the strip-resistance:
(a is None) != (b is None). The new formif (a is None) == (b is None): raise ValueError(...)catches both "neither provided" and "both provided" with the same wording — without needing to mentally parse a!=of two booleans.construct_imagecheck now includes the offending tensor in the error message (f"got {camera_ids.tolist()}"), instead of a bareAssertionErrorthat hides the value.Verification
After this PR,
grep -n "^\s*assert " src/alpamayo_r1/chat_template/conversation.pyreturns nothing. Behavior with optimization off is identical; with optimization on, the checks now actually run.