-
Notifications
You must be signed in to change notification settings - Fork 488
lazy load jailbreak detection dependencies #1223
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
base: develop
Are you sure you want to change the base?
lazy load jailbreak detection dependencies #1223
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.
I'm ok with this. Will need @erickgalinkin to ensure current test coverage is sufficient.
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.
This looks great to me! Thanks for the suggestion @jeffreyscarpenter !
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.
Looks good, could you add tests to ensure the correct modules are imported and the functions still work as before?
@@ -71,6 +73,7 @@ def __call__(self, text: str): | |||
|
|||
class JailbreakClassifier: | |||
def __init__(self, random_forest_path: str): | |||
from sklearn.ensemble import RandomForestClassifier | |||
self.embed = SnowflakeEmbed() | |||
with open(random_forest_path, "rb") as fd: | |||
self.classifier = pickle.load(fd) |
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.
Nit: Should we include a pickle in the init() along with the RandomForestClassifier? My guess is scikit-learn imports it so we pick it up implicitly
|
||
@lru_cache() | ||
def initialize_model(classifier_path: str = models_path) -> JailbreakClassifier: | ||
def initialize_model(classifier_path: str = models_path): |
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.
Nit: Could you add the return -type back in (as 'JailbreakClassifier'
now we're importing this in the function)?
Description
Fixes #1222 by making jailbreak detection dependencies lazy-loaded. This means torch, transformers, and sklearn are only imported when using the local model-based approach, not when using the NIM-based approach.
Related Issue(s)
Fixes #1222
Checklist