-
Notifications
You must be signed in to change notification settings - Fork 178
Update functional.py #419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update functional.py #419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR adds error handling to the infra/modal/functional.py file, enhancing code robustness and debugging capabilities for the Infinity project's Modal deployment.
- Added try-except blocks in
BaseInfinityModelmethodsdownload_modelandenterto catch and log potential errors during model downloading and engine array initialization. - Implemented error handling in
InfinityModalmethodsembed,image_embed,rerank, andclassify, returningNoneif exceptions occur during processing. - Enclosed the
mainfunction's operations in a try-except block to catch and print any errors that may occur during remote method calls. - Consider adding more specific error types and logging mechanisms for better error tracking and debugging.
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| print(f"Error embedding sentences: {e}") | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: returning None silently may lead to unexpected behavior in calling code. Consider raising a custom exception
| classifications_1 if classifications_1 else "N/A", | ||
| ) | ||
| except Exception as e: | ||
| print(f"Error in main entrypoint: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider using a more specific exception type instead of catching all exceptions
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
=======================================
Coverage 79.13% 79.13%
=======================================
Files 40 40
Lines 3173 3173
=======================================
Hits 2511 2511
Misses 662 662 ☔ View full report in Codecov by Sentry. |
michaelfeil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YadlaMani thanks for the improvement. I have not used this in a long time? (2months)
How does it improve the debugging?
Would it be helpful to raise an speicifc error instead of printing and returning None?
Have you tried it out with modal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
This PR modifies error handling in the Modal functional API example, but has some implementation concerns that need to be addressed.
- Returning
Nonesilently inInfinityModalmethods masks failures and makes debugging harder - should raise specific exceptions instead - Generic
Exceptioncatches in try-except blocks are too broad - should catch specific error types likeModelLoadErrororInferenceError - Error messages only use
print()statements - should utilize proper logging with levels (error, warning, etc.) - Missing error propagation mechanism to inform callers of failures in
BaseInfinityModelmethods - No validation that the code works with Modal deployment - needs testing to verify error handling in production environment
The changes add basic error visibility but need refinement to be truly useful for debugging and production use.
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| try: | ||
| print(f"downloading models {self.model_id} ...") | ||
| self._get_array() | ||
| except Exception as e: | ||
| print(f"Error downloading model: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider propagating the error since this is a critical initialization step that should not fail silently
| try: | ||
| print("Starting the engine array ...") | ||
| self.engine_array = self._get_array() | ||
| await self.engine_array.astart() | ||
| print("engine array started!") | ||
| except Exception as e: | ||
| print(f"Error starting the engine array: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: initialization failure here could leave the engine in an invalid state - consider cleanup or proper error propagation
This reverts commit d1fa1c1.
Incorporate error handling to enhance code quality and simplify debugging.