-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add hooks, fickling.load(), and a JSON output format for usability #79
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.
@suhacker1 good job on this! It needs some restructuring but the core idea is there :) I've left a bunch of comments, some are nits, but most of them are about code and architecture. Let's address all of them and then I'll make a second pass on the PR.
Note: We decided not to include the PyTorch global hook in this PR. We also decided to remove the UNKNOWN severity type as we felt it was redundant. |
@Boyan-MILANOV This is ready for another review! |
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.
LGTM now! Good job! 🚀
This PR makes multiple feature additions for usability. Specifically, this PR adds a fickling import hook, global function hook,
fickling.load()
function, and a JSON output format for thecheck_safety
component of the CLI. Each of these features can make it easier to integrate fickling into different codebases and tools.This PR also updates the examples and tests to reflect these new features. Additional important changes made include:
is_likely_safe
infickle.py
withcheck_safety
inanalysis.py
: A newcheck_safety
method was added tofickle.py
as a wrapper withis_likely_safe
being marked for deprecation.analysis.py
: Not only wasProtoAnalysis
split for simplicity, but more structure was added throughout the different analysis classes to enable the reporting of detailed results.pytorch.py
: ThePyTorchModelWrapper
class now reports the identified file formats from the validation method.I would especially appreciate feedback on:
torch.load
should be includedExample JSON Output ("test_unused_variables.json"):