feat(iorails): Add synchronous generate() method - #1654
Conversation
|
@greptile review PR |
Greptile SummaryThis PR adds a synchronous
|
| Filename | Overview |
|---|---|
| nemoguardrails/guardrails/iorails.py | Adds synchronous generate() method that creates a fresh IORails instance per call via asyncio.run(), intentionally trading latency for simplicity in avoiding async/sync loop conflicts. Stores config on the instance to enable this pattern. |
| nemoguardrails/guardrails/guardrails.py | Removes the NotImplementedError for IORails.generate(), simplifies generate() to a polymorphic call to self.rails_engine.generate() without the previous cast to LLMRails. |
| tests/guardrails/test_guardrails.py | Updates routing test to mock and assert IORails.generate() instead of expecting NotImplementedError. Removes previous redundant mock assignment (addressed in prior review thread). |
| tests/guardrails/test_iorails.py | Adds TestGenerate class with three tests: delegation to generate_async via a temp instance, kwargs forwarding, and RuntimeError when called from an existing async loop. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Guardrails
participant IORails as IORails (self)
participant TempIORails as IORails (temp)
participant MM as ModelManager
participant RM as RailsManager
Caller->>Guardrails: generate(prompt/messages)
Guardrails->>IORails: generate(messages, **kwargs)
Note over IORails: asyncio.run(_run_sync_iorails())
IORails->>TempIORails: IORails(self.config)
TempIORails->>MM: start()
TempIORails->>TempIORails: generate_async(messages, **kwargs)
TempIORails->>RM: is_input_safe(messages)
RM-->>TempIORails: RailResult(is_safe=True)
TempIORails->>MM: generate_async("main", messages)
MM-->>TempIORails: response_text
TempIORails->>RM: is_output_safe(messages, response_text)
RM-->>TempIORails: RailResult(is_safe=True)
TempIORails->>MM: stop()
TempIORails-->>IORails: {"role": "assistant", "content": response_text}
IORails-->>Guardrails: LLMMessage
Guardrails-->>Caller: LLMMessage
Last reviewed commit: 1df9c83
Additional Comments (1)
This test expects Either add the fallback logic in self.model_name: str = model_config.model or params.get("model_name", "")or update the test to match the current behavior. Prompt To Fix With AIThis is a comment left during a code review.
Path: tests/guardrails/test_model_engine.py
Line: 179:182
Comment:
**Test asserts behavior that doesn't exist in code**
This test expects `model_name` to fall back to `parameters["model_name"]` when `model` is `None`, but `ModelEngine.__init__` (line 69 of `model_engine.py`) only does `self.model_name: str = model_config.model or ""`. There is no logic to read `model_name` from `parameters`, so `engine.model_name` will be `""`, not `"param-model"`, and this test will fail.
Either add the fallback logic in `ModelEngine.__init__`:
```python
self.model_name: str = model_config.model or params.get("model_name", "")
```
or update the test to match the current behavior.
How can I resolve this? If you propose a fix, please make it concise. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@greptile review PR in light of feedback in this comment |
|
@greptile review with latest commit SHA d4ff6708 |
22f7b60 to
1c5e23e
Compare
5cde0ac to
f3445e4
Compare
|
@greptile review this PR (SHA 0a28bdbf6a) |
Description
This PR is stacked on top of #1649 , which is in turn stacked on top of #1638 .
The LLMRails class has two generation methods, synchronous generate() and asynchronous generate_async(). The IORails engine supports async workflows only as it's designed for high throughput non-blocking requests. It uses an AsyncWorkQueue to buffer requests until they can be processed, which doesn't make sense for a synchronous method.
So the aim of adding generate() to IORails is to implement the simplest functionally compatible implementation, while not optimizing for latency (since generate_async() is recommended for performance). Inter-mixing a synchronous request with asynchronous code causes issues with different asyncio loops used for sync/async requests and internal objects like ModelEngine. To this end, IORails is created on every synchronous generate() call which pays a latency penalty but greatly simplifies the code and ongoing maintenance for generate_sync().
Related Issue(s)
Test Plan
Pre-commit
Unit-test
IPython testing (content-safety)
Checklist