Return ValueError instead of panicking on duplicate mergeable_ranks#579
Open
devYRPauli wants to merge 1 commit into
Open
Return ValueError instead of panicking on duplicate mergeable_ranks#579devYRPauli wants to merge 1 commit into
devYRPauli wants to merge 1 commit into
Conversation
Constructing an Encoding with duplicate ranks in mergeable_ranks made CoreBPE::new_internal hit a raw assert!, which surfaces to Python as an uncatchable pyo3_runtime.PanicException instead of a clean ValueError. new_internal already returns Result and reports regex errors cleanly via Regex::new(pattern)?; py.rs maps that Err to PyValueError. Return an Err on the length mismatch so it flows through the same path. Refs openai#87. Signed-off-by: Yash Raj Pandey <yashpn62@gmail.com>
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.
Problem
Constructing an
Encodingwith duplicate ranks inmergeable_rankscrashes with an uncatchablepyo3_runtime.PanicExceptioninstead of raising a normalValueError:Because it is a panic rather than an exception, callers cannot handle it with
except ValueError(or any normalexcept), and it can tear down the interpreter.Root cause
In
CoreBPE::new_internal(src/lib.rs), the length mismatch between the encoder and the derived decoder - which happens when two tokens share a rank - is checked with a rawassert!.new_internalalready returnsResult<Self, ...>and surfaces regex errors cleanly viaRegex::new(pattern)?, andpy.rsmaps thatErrtoPyValueError. Theassert!panics before it can return through that path.Fix
Return an
Erron the length mismatch so it flows through the existingResultpath (and therefore thePyValueErrormapping inpy.rs) instead of panicking. The message is unchanged.Test
Adds
tests/test_duplicate_ranks.py, which asserts that duplicatemergeable_ranksraiseValueError. Before the change it fails withPanicException; after, it passes.Refs #87 (same crash, reported in 2023).