Repository navigation
guard: expose ml_load_error and test corrupt checkpoints (#191) - #209
Open
AaliyaKhan25 wants to merge 2 commits into
Open
AaliyaKhan25 wants to merge 2 commits into
AaliyaKhan25 wants to merge 2 commits into
Conversation
 [Medium impact] The PR appears safe to merge; the previous finding is fixed and no new issues were found. SummaryAdds
Reviews (2) · Last reviewed commit: "tests: skip checkpoint tests when transf..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
If torch is installed but transformers is not, model loading raises an
install-hint error before config.json is read. Both new checkpoint tests
would then fail on the filename assertion even though Guard behaves
correctly. Guard both with pytest.importorskip('transformers').
Fixes P2 raised in Greptile review.
This branch has not been deployed
This file contains hidden or 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
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.
Summary
Closes #191.
I could not reproduce
ml_model_loadedbeing true for a corrupt checkpoint on currentdev. Both tests named in the issue pass for me (torch 2.12, transformers 4.44.2, Python 3.12, Windows, empty model cache), and they also pass at the commit before #174. The part of the issue that does hold is that the real load error only reached a log line: the warning logs the exception class name, so a caller running withrequire_ml=Falsehad no way to see why ML was off.Guard.ml_load_errorreturns the exception that stopped the model loading, orNoneif it loaded or was never attempted.test_ml_degraded_warning.pypins it.require_ml=False, they assertml_model_loaded is Falseand that the error is exposed. Withrequire_ml=True, they assert theModelErrorcarries the underlying cause.Checklist
devcd sdk && make check-cipasses locallysdk/README.md/docs/Notes for reviewers
ml_load_erroris new public API and overlaps with the degraded reporting in #107, so tell me if you want a different shape. It also keeps the exception's traceback alive for the Guard's lifetime. If you'd rather not, I can storerepr(exc)instead.I ran pytest on the two touched test files, plus
ruff check,ruff format --checkandmypydirectly, becausemakeisn't available on my Windows setup. I did not run the fullmake check-ci.On Windows,
test_require_ml_error_names_the_offending_pathfails on unmodifieddev. The message uses{path!r}, which doubles the backslashes, sostr(bad) in str(exc)can't match. It's unrelated to this change, and Linux CI doesn't see it. I'm happy to send a separate fix once this is merged.Drafted with AI assistance. I traced the load path and ran the repro and tests myself.