fix: Restore explicit error surfacing for unexpected hub exceptions #37525
Conversation
Prior to PR #36033, unexpected exceptions (e.g., ModuleNotFoundError) during hub model loading were not swallowed silently. They either matched specific except blocks or were raised. After #36033, a catch-all except Exception block was introduced without a fallback else, causing unknown errors to be silently ignored and leading to misleading downstream behavior. This commit adds an `else: raise e` to ensure only explicitly handled exceptions are suppressed. All others are surfaced, restoring pre-4.50 behavior and aiding in debugging and dependency visibility.
|
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the |
Rocketknight1
left a comment
Rocketknight1
left a comment
There was a problem hiding this comment.
Yes, nice detective work figuring it out and solving it cleanly!
|
Test issues here may be caused by intermittent Hub issues right now, although the change in this PR may make explicit errors more likely when that happens! You can try just pushing empty commits or rebasing to trigger tests to see if a second run fixes things. |
thanks, youre right. I didnt see there was more condition tests right after :) |
manueldeprada
requested review from
Cyrilvallez
and removed request for
ArthurZucker
There was a problem hiding this comment.
Alright, LGTM now! 🤗 Thanks a lot! Hopefully that will help surface clearer issues, especially with the hub problems these days.
Do you know the range of Exception we expect to re-raise in this final else? Is it only ModuleNotFoundError, or some other types as well?
Let's wait for the CIs, then this should be good to go!
|
Wait, I think this final condition should in fact be elif not isinstance(e, EntryNotFoundError):
raise ebecause as I mentionned in the comment, this specific one is treated later |
thanks!! regarging CI, it's difficult to spot actual errors with the current flood of 404s. Maybe best to wait until the hub issues are resolved before merging? |
I see... took me time to follow that flow. thanks a lot for taking the time to review! 🤗 maybe I should write some proper exception tests? |
There was a problem hiding this comment.
Alright, I think this is finally the correct fix, and the CI seem to agree with me! Indeed if you don't mind, a test with a custom Exception would indeed be nice to make sure we keep handling it correctly in the future! 🤗
Let's add the comment as well 😉
Co-authored-by: Cyril Vallez <[email protected]>
thanks a lot!! will merge and do the tests later this week :) |
|
It's totally fine and I'm happy that we merged this one quickly as it should help our workflows, but it's in general more advisable to add the tests along the fixes themselves! 🤗 Just because they may sometimes surface issues we did not think about, or simply because we may otherwise forget to add them/get absorbed in other things! |
…uggingface#37525) * fix: Restore explicit error surfacing for unexpected hub exceptions Prior to PR huggingface#36033, unexpected exceptions (e.g., ModuleNotFoundError) during hub model loading were not swallowed silently. They either matched specific except blocks or were raised. After huggingface#36033, a catch-all except Exception block was introduced without a fallback else, causing unknown errors to be silently ignored and leading to misleading downstream behavior. This commit adds an `else: raise e` to ensure only explicitly handled exceptions are suppressed. All others are surfaced, restoring pre-4.50 behavior and aiding in debugging and dependency visibility. Co-authored-by: Cyril Vallez <[email protected]>
…uggingface#37525) * fix: Restore explicit error surfacing for unexpected hub exceptions Prior to PR huggingface#36033, unexpected exceptions (e.g., ModuleNotFoundError) during hub model loading were not swallowed silently. They either matched specific except blocks or were raised. After huggingface#36033, a catch-all except Exception block was introduced without a fallback else, causing unknown errors to be silently ignored and leading to misleading downstream behavior. This commit adds an `else: raise e` to ensure only explicitly handled exceptions are suppressed. All others are surfaced, restoring pre-4.50 behavior and aiding in debugging and dependency visibility. Co-authored-by: Cyril Vallez <[email protected]>
Prior to PR #36033, unexpected exceptions (e.g., ModuleNotFoundError) during hub model loading were not swallowed silently. They either matched specific except blocks or were raised.
After #36033, a catch-all except Exception block was introduced without a fallback else, causing unknown errors to be silently ignored and leading to misleading downstream behavior.
This commit adds an
else: raise eto ensure only explicitly handled exceptions are suppressed. All others are surfaced, restoring pre-4.50 behavior and aiding in debugging and dependency visibility.Fixes #37477, see my last comment there for more details.
Since I am new here, I appreciate reviews :)
Should a test for proper exceptions be written so this kind of regression does not happen again?
Who can review?
@gante @Cyrilvallez @Wauplin