Make predict methods in ZeroShot pipeline return Result instead of panicking on unwrap #301
Conversation
This file contains 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
- Add checked prediction methods to ZeroShotClassificationModel. These methods return Option and convert any underlying errors into None, to allow callers to implement appropriate error handling logic.
Rename *_checked methods into try_* methods. This is more idiomatic vis-a-vis the Rust standard library.
anna-hope
changed the title
anna-hope
changed the title
There was a problem hiding this comment.
Thank you very much for the changes @anna-hope , this looks good.
I think it would be OK to change the signature of the current predict and predict_multilabel to return a Result rather than having multiple methods (that just unwrap for the user). The next release will introduce some breaking changes already. I added one minor comment to move the Option to Error conversion to the method that would produce an invalid output (this also saves code duplication since it is called twice)
added 2 commits
November 29, 2022 17:03
Change return type of ZeroShotClassificationModel.prepare_for_model from option into Result. This simplifies the code, and returns the error closer to its origin. This addresses comments from @guillaume-be.
|
@guillaume-be Thank you very much for your feedback! I addressed your comments. This did indeed result in cleaner code. |
anna-hope
changed the title
Miezhiko
pushed a commit
to Masha/rust-bert
that referenced
this pull request
Mar 21, 2023
…nicking on unwrap (guillaume-be#301) * Add checked prediction methods - Add checked prediction methods to ZeroShotClassificationModel. These methods return Option and convert any underlying errors into None, to allow callers to implement appropriate error handling logic. * Update ZeroShot example to use checked method. * Add tests for ZeroShot checked methods * Change checked prediction methods to return Result * refactor: rename *_checked into try_* Rename *_checked methods into try_* methods. This is more idiomatic vis-a-vis the Rust standard library. * refactor: remove try_ prefix from predict methods * refactor: change return from Option to Result Change return type of ZeroShotClassificationModel.prepare_for_model from option into Result. This simplifies the code, and returns the error closer to its origin. This addresses comments from @guillaume-be. * refactor: address clippy lints in tests Co-authored-by: guillaume-be <guillaume.becquin@gmail.com>
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.
Add checked prediction methods to ZeroShotClassificationModel.
These methods return
Resultto allow callers to implement appropriate error handling logic.I named these methods
try_by analogy withtry_methods inHashMapand elsewhere in the standard library.This closes #297 for ZeroShotClassificationModel.