Improve error handling for neural model and tokenizer loading#472
Improve error handling for neural model and tokenizer loading#472chimchim89 wants to merge 3 commits intochonkie-inc:mainfrom
Conversation
Summary of ChangesHello @chimchim89, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the error handling within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a good step towards improving error handling for model and tokenizer loading. Using specific exceptions like OSError and ValueError is much better than a generic Exception. However, the implementation has a couple of issues. The new except ValueError blocks are too broad and accidentally catch more specific, developer-raised ValueErrors, replacing helpful error messages with generic ones. This regresses the debugging experience for certain user errors. I've left detailed comments on how to address this by restructuring the validation and loading logic. There's also a minor suggestion to improve the clarity of an error message in the OSError handler.
src/chonkie/chunker/neural.py
Outdated
| except ValueError as e: | ||
| raise ValueError( | ||
| "The provided tokenizer is invalid or incompatible with the model." | ||
| ) from e |
There was a problem hiding this comment.
This except ValueError block is too broad. It unintentionally catches the ValueError raised on line 98 when an invalid tokenizer type is provided. This replaces a specific and helpful error message with a more generic one, which can make debugging harder for users. It's recommended to refactor the logic to separate the input type validation from the exception handling for loading errors. The try...except block should ideally only wrap the calls to AutoTokenizer.from_pretrained.
src/chonkie/chunker/neural.py
Outdated
| except ValueError as e: | ||
| raise ValueError( | ||
| "The model is not compatible with token-classification." | ||
| ) from e |
There was a problem hiding this comment.
Similar to the tokenizer handling, this except ValueError block is too broad. It catches specific ValueError exceptions raised for an invalid model type (line 135) or an unsupported model (line 120). This replaces useful, specific error messages with a generic one, hindering debugging. Consider restructuring the code to handle type validation, support checks, and loading errors separately so that this except block only catches loading-related errors from from_pretrained.
src/chonkie/chunker/neural.py
Outdated
| raise ValueError( | ||
| f"Failed to load tokenizer '{tokenizer}'. " | ||
| "Check that the tokenizer name is correct." | ||
| ) from e |
There was a problem hiding this comment.
The error message here can be misleading. When tokenizer is None, the code attempts to load it from the model string. If that fails, this message will display ... 'None' ..., which is not helpful. The message should indicate which identifier failed to load.
| raise ValueError( | |
| f"Failed to load tokenizer '{tokenizer}'. " | |
| "Check that the tokenizer name is correct." | |
| ) from e | |
| raise ValueError( | |
| f"Failed to load tokenizer '{model if tokenizer is None else tokenizer}'. " | |
| "Check that the tokenizer name is correct." | |
| ) from e |
1 participant
This PR makes error messages around model and tokenizer loading a bit clearer in NeuralChunker.
Instead of catching everything with a generic exception, it now raises more specific and helpful errors when:
a model or tokenizer can’t be loaded
an invalid or incompatible model/tokenizer is passed in
No behavior changes this just makes failures easier to understand and debug.