refactor: Guardrails public API#1933
Open
tgasser-nv wants to merge 9 commits into
Open
Conversation
…ails, and IORails
…params} attributes to private, add deprecation warning for public accesses
…th read-only state in next release
…into private has-a objects
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
Greptile SummaryThis PR refactors the NeMo Guardrails public API by introducing a
|
| Filename | Overview |
|---|---|
| nemoguardrails/base_guardrails.py | New ABC defining the minimal shared contract; correctly uses @AbstractMethod for generate, generate_async, and stream_async; config annotation is informational only (not ABC-enforced), which the docstring correctly documents. |
| nemoguardrails/rails/llm/llmrails.py | Reclassifies kb, embedding_search_providers, default_embedding_, explain_info, and llm_generation_actions to private with deprecated getters (and setters for the embedding_ and explain_info attributes); adds first-class passthrough_fn property/setter; all internal usages correctly updated to use private names. |
| nemoguardrails/guardrails/guardrails.py | Adds explain_info (deprecated), passthrough_fn, and events_history_cache proxies to the Guardrails facade; IORails guard consistently raised before any side effects; delegation to LLMRails private attributes is direct (avoids double-warning). |
| nemoguardrails/guardrails/iorails.py | Trivial change: IORails now inherits from BaseGuardrails; no logic changes. |
| nemoguardrails/integrations/langchain/runnable_rails.py | Updated to use the new first-class rails.passthrough_fn API instead of rails.llm_generation_actions.passthrough_fn. |
| tests/guardrails/test_public_api_deprecations.py | New comprehensive test suite covering all deprecated aliases (read + write), passthrough_fn first-class property, and Guardrails facade proxies; uses LLMRails.new and patched init correctly to isolate descriptor behavior. |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
class BaseGuardrails {
<<abstract>>
+RailsConfig config
+generate()*
+generate_async()*
+stream_async()*
}
class LLMRails {
+RailsConfig config
+LLMModel llm
+Runtime runtime
+passthrough_fn [property/setter]
-_kb
-_embedding_search_providers
-_explain_info
-_llm_generation_actions
+kb [deprecated getter]
+embedding_search_providers [deprecated getter]
+default_embedding_model [deprecated getter/setter]
+explain_info [deprecated getter/setter]
+llm_generation_actions [deprecated getter]
+generate()
+generate_async()
+stream_async()
+explain()
}
class IORails {
+RailsConfig config
+generate()
+generate_async()
+stream_async()
}
class Guardrails {
+RailsConfig config
-_rails_engine
+rails_engine [property]
+passthrough_fn [property/setter]
+explain_info [deprecated getter/setter]
+events_history_cache [property/setter]
+generate()
+generate_async()
+stream_async()
+explain()
}
class LLMGenerationActions {
-_passthrough_fn
}
BaseGuardrails <|-- LLMRails
BaseGuardrails <|-- IORails
BaseGuardrails <|-- Guardrails
Guardrails --> LLMRails : wraps (use_iorails=False)
Guardrails --> IORails : wraps (use_iorails=True)
LLMRails --> LLMGenerationActions : _llm_generation_actions
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
nemoguardrails/rails/llm/llmrails.py:143-150
**`kb` deprecation message lacks migration guidance**
The `embedding_search_providers` deprecation message helpfully says _"use `register_embedding_search_provider()` to add providers"_, while `kb` just says it is deprecated with no hint of what to do. A user who reads the warning and wants to stop using `rails.kb` has nowhere to turn. Consider adding a note such as _"There is no public read API for the knowledge base; interact with it through the action system."_ (or similar), so the migration path is clear before the attribute is removed.
### Issue 2 of 2
nemoguardrails/guardrails/guardrails.py:168-186
**`Guardrails` facade omits deprecated proxies for `kb`, `embedding_search_providers`, `default_embedding_*`, and `llm_generation_actions`**
The PR description states that _"every attribute `LLMRails` exposes has to be handled by `Guardrails`"_ (since `NEMO_GUARDRAILS_IORAILS_ENGINE=1` aliases `LLMRails` to `Guardrails`). Four deprecated-but-still-present properties on `LLMRails` — `kb`, `embedding_search_providers`, `default_embedding_model/engine/params`, and `llm_generation_actions` — are not proxied on the `Guardrails` facade. A caller using the env-var alias who accesses any of these will get a bare `AttributeError` instead of the deprecation warning they would get with a direct `LLMRails` instance. Adding deprecated getter (and setter) proxies to `Guardrails` for these attributes, guarded with the same `isinstance(self.rails_engine, IORails): raise NotImplementedError` pattern, would make the alias behaviour consistent with the PR's stated goal.
Reviews (4): Last reviewed commit: "Add comments to address greptile feedbac..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughThis PR refactors the NeMo Guardrails codebase to establish a common abstract base interface ( ChangesPublic API Refactoring for Guardrails Engine Interface
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…arts with test and is picked up by pytest
Collaborator
|
@tgasser-nv can we hold this until we get e2e tests and LLMRails refactor draft ready? I have some ideas about this. |
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
2 participants
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.
Description
Create
BaseGuardrailsbase class to define public API (with very loose typing for now). UnderNEMO_GUARDRAILS_IORAILS_ENGINE=1,LLMRailsis aliased toGuardrails, so every attributeLLMRailsexposes has to be handled byGuardrails.Several attributes were leaking that shouldn't have been public, and the facade was missing a few proxies. Internal attributes are now
_underscorewith deprecated@propertyaliases; real public knobs are first-class@property/@setteron bothLLMRailsandGuardrails(facade raises under IORails).Follow-on PRs
Changes
BaseGuardrailsabstract base class (nemoguardrails/base_guardrails.py) — minimal contract (generate,generate_async,stream_async,config) shared byLLMRails,IORails, andGuardrails.@propertyaliases:kb,embedding_search_providers,default_embedding_{model,engine,params},llm_generation_actions,explain_info.LLMGenerationActions.passthrough_fn→_passthrough_fn(storage now fully internal).passthrough_fnto a first-class@property+@setteronLLMRailsandGuardrails— replaces the previous attribute on LLMGenerationActions (which is a private object).rails.llm_generation_actions.passthrough_fn = ...would have raisedAttributeErrorwhen using Guardrails.explain_infoproxy onGuardrails(deprecated, points users toexplain()); raises under IORails.events_history_cacheproxy onGuardrails(@property+@setter, raises under IORails).LLMRails.events_history_cachestays a plain public attribute — the server tightly couples to it for now.rails.passthrough_fnAPI.Related Issue(s)
NVBug 6102155
NGUARD-770
Test Plan
Pre-commit
Unit-test
Integration test with Chat (IORails)
Integration test with Chat (LLMRails)
Checklist