fix RecursionError on repeated phrases in bunsetu recognition - #265
Open
Bharath-970 wants to merge 1 commit into
Open
fix RecursionError on repeated phrases in bunsetu recognition#265Bharath-970 wants to merge 1 commit into
Bharath-970 wants to merge 1 commit into
Conversation
A long run of the same repeated token (e.g. '漢字'*1000) builds a dependency chain deep enough to blow past Python's recursion limit. Turns out there are two separate recursive tree walks in this file that both hit this: _traverse() inside bunsetu_phrase_span(), and _children_except_clause_heads() inside BunsetuRecognizer. The issue points at the second one, but the first one actually crashes first since it runs earlier in the pipeline. Rewrote both as explicit-stack loops instead of just raising the recursion limit, since that's an actual fix rather than pushing the ceiling further out. Checked both are behaviorally identical to the recursive originals by running them against 3000+ random tree shapes each and diffing the output, and confirmed identical bunsetu output on normal Japanese sentences before/after. Fixes megagonlabs#261
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.
Was looking into #261 and it turns out the crash isn't even coming from the function the issue points at. There's a second, nearly identical recursive tree walk earlier in the same file (
_traverseinsidebunsetu_phrase_span) that hits the same wall and crashes first. Fixed both while I was in there.They're both just plain recursion over a token's dependency children, and a long run of the same repeated token builds a chain deep enough to blow past Python's default recursion limit. I rewrote both as explicit-stack loops rather than bumping
sys.setrecursionlimit, that would've just moved the ceiling instead of fixing anything.Before touching the real code I wanted to make sure I wasn't quietly changing behavior, so I wrote a small script that ran the old recursive version and the new iterative one against 3000+ randomly generated tree shapes each and compared the output. No mismatches on either function. Then I actually reproduced the bug: installed
ja_ginza, ran the exact'漢字'*1000example from the issue, got the sameRecursionError. Dropped the fix in, same input parses fine now. Ran a handful of normal Japanese sentences through both versions too and got identical results.Ran the full test suite (
ja_ginzaandja_ginza_electra, torch and all): 260 passed, 1 failed, and that one needs real GPU hardware just to run, unrelated to any of this.Fixes #261