fix(iorails): Add LLMRails llm and runtime getters#1888
Conversation
tgasser-nv
changed the base branch from
develop
to
fix/guardrails-llmrails-methods
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
tgasser-nv
changed the title
llm and runtime getters
|
@greptile-apps Review this PR |
|
@coderabbitai Review this PR |
|
✅ Actions performedReview triggered.
|
Greptile SummaryThis PR adds
|
| Filename | Overview |
|---|---|
| nemoguardrails/guardrails/guardrails.py | Adds llm and runtime property getters using the established IORails guard + cast pattern; import of Runtime base class is correct. |
| tests/guardrails/test_guardrails.py | New TestGuardrailsAttributes class covers delegation, read-through after update_llm, and NotImplementedError for both IORails cases; no issues found. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Guardrails
participant LLMRails
participant IORails
Caller->>Guardrails: .llm
alt rails_engine is IORails
Guardrails-->>Caller: raise NotImplementedError
else rails_engine is LLMRails
Guardrails->>LLMRails: .llm
LLMRails-->>Guardrails: Optional[LLMModel]
Guardrails-->>Caller: Optional[LLMModel]
end
Caller->>Guardrails: .runtime
alt rails_engine is IORails
Guardrails-->>Caller: raise NotImplementedError
else rails_engine is LLMRails
Guardrails->>LLMRails: .runtime
LLMRails-->>Guardrails: Runtime
Guardrails-->>Caller: Runtime
end
Reviews (1): Last reviewed commit: "Add getters for Guardrails properties: l..." | Re-trigger Greptile
Greptile SummaryThis PR adds
|
| Filename | Overview |
|---|---|
| nemoguardrails/guardrails/guardrails.py | Adds llm and runtime property getters that delegate to LLMRails or raise NotImplementedError for IORails; relies on a negative-only isinstance guard before casting to LLMRails. |
| tests/guardrails/test_guardrails.py | Adds TestGuardrailsAttributes with 7 tests covering delegation, NotImplementedError on IORails, and config accessibility; one test pre-sets the mock attribute before calling update_llm, weakening its "no caching" claim. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Guardrails
participant IORails
participant LLMRails
Caller->>Guardrails: guardrails.llm (or .runtime)
alt rails_engine is IORails
Guardrails-->>Caller: raise NotImplementedError
else rails_engine is LLMRails
Guardrails->>LLMRails: cast + read .llm / .runtime
LLMRails-->>Guardrails: value
Guardrails-->>Caller: return value
end
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:95-108
**Implicit LLMRails assumption via negative-only check**
Both properties guard against `IORails` but then unconditionally `cast` the result to `LLMRails`. If a third engine type is introduced in the future, the `cast` would silently succeed (no-op at runtime) and the attribute access would either return a wrong value or raise an `AttributeError` with a confusing message. A positive `isinstance(self.rails_engine, LLMRails)` check would make the assumption explicit and produce a clear error for unsupported engines.
### Issue 2 of 2
tests/guardrails/test_guardrails.py:795-812
**`update_llm` call is inconsequential to the assertion**
The test sets `mock_llmrails_instance.llm = new_llm` directly *before* calling `guardrails.update_llm(new_llm)`, so the subsequent `assert guardrails.llm is new_llm` would pass regardless of whether `update_llm` on the `Guardrails` facade actually delegates to `LLMRails`. To genuinely prove "no caching on the facade", the manual attribute assignment should come *after* the `update_llm` call and the test should verify `mock_llmrails_instance.update_llm.assert_called_once_with(new_llm)`.
Reviews (2): Last reviewed commit: "Add getters for Guardrails properties: l..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if isinstance(self.rails_engine, IORails): | ||
| raise NotImplementedError("IORails doesn't support llm attribute access") | ||
|
|
||
| llmrails = cast(LLMRails, self.rails_engine) | ||
| return llmrails.llm | ||
|
|
||
| @property | ||
| def runtime(self) -> Runtime: | ||
| """The Colang runtime backing the rails engine. Only supported for LLMRails.""" | ||
| if isinstance(self.rails_engine, IORails): | ||
| raise NotImplementedError("IORails doesn't support runtime attribute access") | ||
|
|
||
| llmrails = cast(LLMRails, self.rails_engine) | ||
| return llmrails.runtime |
There was a problem hiding this comment.
Both properties guard against IORails but then unconditionally cast the result to LLMRails. If a third engine type is introduced in the future, the cast would silently succeed (no-op at runtime) and the attribute access would either return a wrong value or raise an AttributeError with a confusing message. A positive isinstance(self.rails_engine, LLMRails) check would make the assumption explicit and produce a clear error for unsupported engines.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/guardrails/guardrails.py
Line: 95-108
Comment:
**Implicit LLMRails assumption via negative-only check**
Both properties guard against `IORails` but then unconditionally `cast` the result to `LLMRails`. If a third engine type is introduced in the future, the `cast` would silently succeed (no-op at runtime) and the attribute access would either return a wrong value or raise an `AttributeError` with a confusing message. A positive `isinstance(self.rails_engine, LLMRails)` check would make the assumption explicit and produce a clear error for unsupported engines.
How can I resolve this? If you propose a fix, please make it concise.| assert guardrails.llm is new_llm | ||
|
|
||
| @patch("nemoguardrails.guardrails.guardrails.LLMRails") | ||
| def test_config_attribute_on_llmrails(self, mock_llmrails_class, _nemoguards_rails_config): | ||
| """guardrails.config is the same RailsConfig instance passed in.""" | ||
| mock_llmrails_class.return_value = MagicMock() | ||
| guardrails = Guardrails(config=_nemoguards_rails_config, use_iorails=False) | ||
| assert guardrails.config is _nemoguards_rails_config | ||
|
|
||
| @patch.object(IORails, "__init__", return_value=None) | ||
| def test_llm_property_raises_on_iorails(self, mock_iorails_init, _content_safety_rails_config): | ||
| """guardrails.llm raises NotImplementedError when running on IORails.""" | ||
| guardrails = Guardrails(config=_content_safety_rails_config, use_iorails=True) | ||
| assert isinstance(guardrails.rails_engine, IORails) | ||
|
|
||
| with pytest.raises(NotImplementedError, match="IORails doesn't support llm attribute access"): | ||
| _ = guardrails.llm | ||
|
|
There was a problem hiding this comment.
update_llm call is inconsequential to the assertion
The test sets mock_llmrails_instance.llm = new_llm directly before calling guardrails.update_llm(new_llm), so the subsequent assert guardrails.llm is new_llm would pass regardless of whether update_llm on the Guardrails facade actually delegates to LLMRails. To genuinely prove "no caching on the facade", the manual attribute assignment should come after the update_llm call and the test should verify mock_llmrails_instance.update_llm.assert_called_once_with(new_llm).
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/guardrails/test_guardrails.py
Line: 795-812
Comment:
**`update_llm` call is inconsequential to the assertion**
The test sets `mock_llmrails_instance.llm = new_llm` directly *before* calling `guardrails.update_llm(new_llm)`, so the subsequent `assert guardrails.llm is new_llm` would pass regardless of whether `update_llm` on the `Guardrails` facade actually delegates to `LLMRails`. To genuinely prove "no caching on the facade", the manual attribute assignment should come *after* the `update_llm` call and the test should verify `mock_llmrails_instance.update_llm.assert_called_once_with(new_llm)`.
How can I resolve this? If you propose a fix, please make it concise.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds two new read-only properties to the ChangesGuardrails property exposure
🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
1 participant
This PR has a typo in its branch name, see #1889 for the corrected branch name