fix(iorails): Add LLMRails llm and runtime getters - #1889
Conversation
|
@greptile-apps Review this PR |
|
@coderabbitai Review this PR |
|
✅ Actions performedReview triggered.
|
Greptile SummaryAdds
|
| Filename | Overview |
|---|---|
| nemoguardrails/guardrails/guardrails.py | Adds llm and runtime properties with correct IORails guard and cast delegation; also imports Runtime for the return-type annotation. |
| tests/guardrails/test_guardrails.py | Adds TestGuardrailsAttributes class with seven focused tests covering property delegation, post-update_llm read-through, and NotImplementedError on IORails. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Guardrails
participant LLMRails
participant IORails
Caller->>Guardrails: .llm
alt engine is LLMRails
Guardrails->>LLMRails: .llm
LLMRails-->>Guardrails: Optional[LLMModel]
Guardrails-->>Caller: Optional[LLMModel]
else engine is IORails
Guardrails-->>Caller: NotImplementedError
end
Caller->>Guardrails: .runtime
alt engine is LLMRails
Guardrails->>LLMRails: .runtime
LLMRails-->>Guardrails: Runtime
Guardrails-->>Caller: Runtime
else engine is IORails
Guardrails-->>Caller: NotImplementedError
end
Reviews (4): Last reviewed commit: "Add getters for Guardrails properties: l..." | Re-trigger Greptile
📝 WalkthroughWalkthroughThe PR extends the ChangesGuardrails LLMRails delegation facade
Sequence DiagramsequenceDiagram
participant Client
participant Guardrails
participant LLMRails
participant IORails
Client->>Guardrails: llm property
alt use_iorails_engine is False
Guardrails->>LLMRails: return llm
LLMRails-->>Guardrails: LLMModel
Guardrails-->>Client: LLMModel
else use_iorails_engine is True
Guardrails-->>Client: NotImplementedError
end
Client->>Guardrails: generate_events(events)
alt use_iorails_engine is False
Guardrails->>LLMRails: generate_events(events)
LLMRails-->>Guardrails: result events
Guardrails-->>Client: result events
else use_iorails_engine is True
Guardrails-->>Client: NotImplementedError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoguardrails/guardrails/guardrails.py (1)
398-412: 💤 Low valueConsider documenting that
verboseis also dropped during pickling.The docstring mentions that
llmis dropped, butverboseis also not preserved (hardcoded toFalsein__setstate__). This is likely intentional since verbose controls logging configuration at construction time, but documenting it would clarify the behavior.📝 Suggested documentation improvement
def __getstate__(self): """Pickle support: preserve config and use_iorails so the rebuilt - instance lands on the same engine. The llm is dropped (matches LLMRails). + instance lands on the same engine. The llm and verbose are dropped + (matches LLMRails behavior; verbose defaults to False on restore). """ return {"config": self.config, "use_iorails": self.use_iorails_engine}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoguardrails/guardrails/guardrails.py` around lines 398 - 412, The pickling methods __getstate__ and __setstate__ drop the instance's verbose setting (verbose is hardcoded to False on restore) but the docstring only mentions llm is dropped; update the docstring for __getstate__ and/or __setstate__ to explicitly state that verbose is not preserved across pickling and that restored instances are constructed with verbose=False (or document the backward-compatible behavior), referencing __getstate__, __setstate__, and the verbose parameter so readers know logging/verbosity is intentionally not retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@nemoguardrails/guardrails/guardrails.py`:
- Around line 398-412: The pickling methods __getstate__ and __setstate__ drop
the instance's verbose setting (verbose is hardcoded to False on restore) but
the docstring only mentions llm is dropped; update the docstring for
__getstate__ and/or __setstate__ to explicitly state that verbose is not
preserved across pickling and that restored instances are constructed with
verbose=False (or document the backward-compatible behavior), referencing
__getstate__, __setstate__, and the verbose parameter so readers know
logging/verbosity is intentionally not retained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81a8a6a4-15a9-41bd-a617-43017fb10152
📒 Files selected for processing (2)
nemoguardrails/guardrails/guardrails.pytests/guardrails/test_guardrails.py
Greptile SummaryThis PR adds
|
| Filename | Overview |
|---|---|
| nemoguardrails/guardrails/guardrails.py | Adds llm/runtime property getters plus 10+ LLMRails-only delegate methods and __getstate__/__setstate__ pickle support; verbose is silently dropped during pickle round-trips. |
| tests/guardrails/test_guardrails.py | Adds three comprehensive test classes covering the new attribute accessors, LLMRails-only delegate methods, and pickle round-trip behaviour including backward-compat scenarios. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Guardrails.llm / .runtime (new properties)"] --> B{rails_engine is IORails?}
B -- Yes --> C[raise NotImplementedError]
B -- No --> D[cast to LLMRails]
D --> E[return llmrails.llm / .runtime]
F["Guardrails.__getstate__"] --> G["{config, use_iorails_engine}"]
G --> H["pickle stream"]
H --> I["Guardrails.__setstate__"]
I --> J{config_path set?}
J -- Yes --> K["RailsConfig.from_path(path)"]
J -- No --> L[use pickled config]
K --> M["self.__init__(config, verbose=False, use_iorails)"]
L --> M
Comments Outside Diff (2)
-
nemoguardrails/guardrails/guardrails.py, line 398-402 (link)verboseis not included in__getstate__, so__setstate__always rebuilds withverbose=False. AGuardrailsinstance created withverbose=Truewill silently lose that setting after a pickle round-trip, which can be hard to diagnose when verbose logging suddenly stops appearing.Prompt To Fix With AI
This is a comment left during a code review. Path: nemoguardrails/guardrails/guardrails.py Line: 398-402 Comment: `verbose` is not included in `__getstate__`, so `__setstate__` always rebuilds with `verbose=False`. A `Guardrails` instance created with `verbose=True` will silently lose that setting after a pickle round-trip, which can be hard to diagnose when verbose logging suddenly stops appearing. How can I resolve this? If you propose a fix, please make it concise.
-
nemoguardrails/guardrails/guardrails.py, line 412 (link)If
verboseis added to__getstate__(see above),__setstate__should restore it rather than hardcodingFalse. Without this, the fix to__getstate__alone would have no effect.Prompt To Fix With AI
This is a comment left during a code review. Path: nemoguardrails/guardrails/guardrails.py Line: 412 Comment: If `verbose` is added to `__getstate__` (see above), `__setstate__` should restore it rather than hardcoding `False`. Without this, the fix to `__getstate__` alone would have no effect. How can I resolve this? If you propose a fix, please make it concise.
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/guardrails/guardrails.py:398-402
`verbose` is not included in `__getstate__`, so `__setstate__` always rebuilds with `verbose=False`. A `Guardrails` instance created with `verbose=True` will silently lose that setting after a pickle round-trip, which can be hard to diagnose when verbose logging suddenly stops appearing.
```suggestion
def __getstate__(self):
"""Pickle support: preserve config, verbose, and use_iorails so the rebuilt
instance lands on the same engine. The llm is dropped (matches LLMRails).
"""
return {"config": self.config, "verbose": self.verbose, "use_iorails": self.use_iorails_engine}
```
### Issue 2 of 2
nemoguardrails/guardrails/guardrails.py:412
If `verbose` is added to `__getstate__` (see above), `__setstate__` should restore it rather than hardcoding `False`. Without this, the fix to `__getstate__` alone would have no effect.
```suggestion
self.__init__(config=config, verbose=state.get("verbose", False), use_iorails=state.get("use_iorails", True))
```
Reviews (2): Last reviewed commit: "Add getters for Guardrails properties: l..." | Re-trigger Greptile
tgasser-nv
changed the title
llm and runtime getters
4ac7bdf to
5f14e49
Compare
|
@greptile-apps I fixed the verbose-not-being pickled issue in the PR before this one in the stack (#1886 ) as it's out-of-scope for this one |
|
That makes sense — keeping the fix in #1886 is the right call since it's where the pickling logic lives. Thanks for the context! |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
5f14e49 to
4dbe80a
Compare
2 participants
Description
Stacked PR: Stacked on top of #1886 . Please review #1886 first before this one.
LLMRails has three public attributes:
*
config: RailsConfig: The RailsConfig configuration used by LLMRails.llm: Optional[LLMModel]: An optional LLM which can be passed into LLMRails rather than constructed from a config.runtime: Runtime: The Colang runtime inside LLMRails that is used to schedule workflows for input, output, dialog, etc rails.This PR adds getters for the
llmandruntimeattributes.There's no setter for the
llmattribute since there's already anupdate_llmmethod which acts as a setter.And no setter for
runtimesince that's fixed by the RailsConfig itself, and can't be changed once the LLMRails object is constructed.Related Issue(s)
Fixes NGUARD-771
Fixes NGUARD-770
Test Plan
Pre-commit
Unit-test
Integration test with Chat
Checklist
Summary by CodeRabbit
New Features
Guardrailsclass now exposes read-onlyllmandruntimepropertiesTests