-
Notifications
You must be signed in to change notification settings - Fork 583
fix(llmrails): move LLM isolation setup to after KB initialization #1348
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
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.
Pull Request Overview
Fixes a timing issue where LLM isolation setup was called too early in initialization, causing flow matching to fail when trying to resolve rail flow IDs. The fix moves the LLM isolation call to after knowledge base initialization to ensure all components are properly set up.
- Moved
_create_isolated_llms_for_actions()call from_init_llms()to after KB setup in__init__() - Added error handling for flow matching failures with fallback to all actions requiring LLMs
- Updated tests to remove prefix matching functionality and test the timing fix scenario
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| nemoguardrails/rails/llm/llmrails.py | Moves LLM isolation setup and adds error handling for flow matching |
| nemoguardrails/rails/llm/utils.py | Removes prefix matching logic from flow ID resolution |
| tests/test_rails_llm_utils.py | Updates tests to remove prefix matching test cases |
| tests/test_llm_isolation.py | Adds test for timing issue with empty flows scenario |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
4a0bb86 to
4971484
Compare
The LLM isolation code was called too early in __init__, before all components were fully initialized. This caused flow matching to fail when trying to resolve rail flow IDs. Move _create_isolated_llms_for_actions() call to after KB setup to ensure all initialization is complete before creating isolated LLMs.
4971484 to
1fd71cc
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1348 +/- ##
========================================
Coverage 71.59% 71.59%
========================================
Files 168 168
Lines 16861 16862 +1
========================================
+ Hits 12071 12072 +1
Misses 4790 4790
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
tgasser-nv
left a comment
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 added a couple of questions, could you take a look before merging? Also could you run a local test with a config that gave the error before the fix, and make sure it's fixed after this PR will be merged?
on develop branch: from nemoguardrails import LLMRails, RailsConfig
config = RailsConfig.from_path("./examples/configs/nemoguards")
rails = LLMRails(config)after the fix: from nemoguardrails import LLMRails, RailsConfig
config = RailsConfig.from_path("./examples/configs/nemoguards")
rails = LLMRails(config) |
…1348) * fix(llmrails): move LLM isolation setup to after KB initialization The LLM isolation code was called too early in __init__, before all components were fully initialized. This caused flow matching to fail when trying to resolve rail flow IDs. Move _create_isolated_llms_for_actions() call to after KB setup to ensure all initialization is complete before creating isolated LLMs.
…1348) * fix(llmrails): move LLM isolation setup to after KB initialization The LLM isolation code was called too early in __init__, before all components were fully initialized. This caused flow matching to fail when trying to resolve rail flow IDs. Move _create_isolated_llms_for_actions() call to after KB setup to ensure all initialization is complete before creating isolated LLMs.
Description
The LLM isolation code was called too early in
__init__, before all components were fully initialized. This caused flow matching to fail when trying to resolve rail flow IDs. This issue was introduce in #1342Move
_create_isolated_llms_for_actions()call to after KB setup to ensure all initialization is complete before creating isolated LLMs.