- 
                Notifications
    
You must be signed in to change notification settings  - Fork 557
 
fix: 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
fix: 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?
        
          
                nemoguardrails/library/jailbreak_detection/model_based/checks.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
      
          
 tests added  | 
    
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.
        
          
                nemoguardrails/library/jailbreak_detection/model_based/checks.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | 
           Thank you @jeffreyscarpenter. Would you please install and run pre-commits and also sign your previous commits?  | 
    
cb90420    to
    8e3220e      
    Compare
  
    
          
 How does it look now @Pouyanpi?  | 
    
          Documentation preview | 
    
          Codecov ReportAttention: Patch coverage is  
 
 Additional details and impacted files@@             Coverage Diff             @@
##           develop    #1223      +/-   ##
===========================================
+ Coverage    68.65%   68.95%   +0.30%     
===========================================
  Files          161      161              
  Lines        15978    15976       -2     
===========================================
+ Hits         10969    11017      +48     
+ Misses        5009     4959      -50     
 Flags with carried forward coverage won't be shown. Click here to find out more. 
 🚀 New features to boost your workflow:
  | 
    
        
          
                nemoguardrails/library/jailbreak_detection/model_based/models.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                nemoguardrails/library/jailbreak_detection/model_based/models.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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.
Thank you @jeffreyscarpenter for your PR, everything looks good 👍🏻 (I just made some minor changes)
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