Skip to content

fix: bare except → except Exception in evaluate.py#109

Open
haosenwang1018 wants to merge 1 commit into
sapientinc:mainfrom
haosenwang1018:fix/bare-except
Open

fix: bare except → except Exception in evaluate.py#109
haosenwang1018 wants to merge 1 commit into
sapientinc:mainfrom
haosenwang1018:fix/bare-except

Conversation

@haosenwang1018

Copy link
Copy Markdown

Replace bare except: with except Exception: in evaluation metric parsing.

@JiwaniZakir JiwaniZakir left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix in evaluate.py is a meaningful improvement over a bare except:, since that would silently swallow KeyboardInterrupt, SystemExit, and other BaseException subclasses. That said, except Exception is still broader than necessary here — the expected failure when loading a torch.compiled checkpoint is almost a RuntimeError or KeyError from load_state_dict mismatched keys, so narrowing to except (RuntimeError, KeyError) would make the intent explicit and prevent unrelated exceptions from being quietly routed to the fallback path.

There's also no logging inside the except block, so if something unexpected causes the first load_state_dict to fail (e.g., a corrupted checkpoint, a CUDA OOM), the code silently retries with the prefix-stripped variant, likely producing a confusing error from the second call rather than a clear message about the root cause. Adding even a logging.warning or print noting that the key-stripping fallback was triggered would help with debugging. The second load_state_dict call itself is also unguarded, so a failure there will surface as an unhandled exception regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants