-
Notifications
You must be signed in to change notification settings - Fork 548
chore(types): Type-clean rails (86 errors) #1396
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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Added some suggestions.
Thank you @tgasser-nv , added some comments and one larger comment in form of a PR, please also have a look at #1410. |
…s mandatory argument as all 2 calls check for None
7595f9c
to
6948258
Compare
I merged PR1410 in as SHA |
… branch" This reverts commit 71d00f0.
…s mandatory argument as all 2 calls check for None
704b6d4
to
5e46e5a
Compare
… options, not gen_options)
0a2639c
to
f0bc730
Compare
5b46a5e
to
d9c0358
Compare
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.
Thank you for adding it to pre-commits, look good to merge 👍🏻
- Fixed 86 type errors across 7 files in the nemoguardrails/rails/ directory. All fixes preserve existing functionality while improving type safety. - added Pyright to pre-commits
- Fixed 86 type errors across 7 files in the nemoguardrails/rails/ directory. All fixes preserve existing functionality while improving type safety. - added Pyright to pre-commits
Description
Cleaned the rails/ directory with help from Cursor/Claude 4 Sonnet to get the low-hanging items.
Type Error Fixes Summary Report
Overview
Fixed 86 type errors across 7 files in the
nemoguardrails/rails/
directory. All fixes preserve existing functionality while improving type safety.Risk Assessment
🔴 High Risk (0 fixes)
No high-risk changes were made.
🟡 Medium Risk (3 fix categories)
1. LLM Generation Actions Constructor Changes
Files:
nemoguardrails/actions/llm/generation.py
Lines: 86
Original Error:
Argument of type "BaseLLM | BaseChatModel | None" cannot be assigned to parameter "llm"
Fixed Code:
Fix Explanation: Modified constructor to accept
Optional
LLM parameter. This allows the system to handle cases where no LLM is provided during initialization. Assumption: The class can gracefully handle None LLM values in its methods. Alternative: Could have added conditional initialization inllmrails.py
, but making the constructor more flexible is cleaner.2. Async Generator Return Type Annotations
Files:
nemoguardrails/rails/llm/buffer.py
Lines: 114-119, 143-146, 259-264
Original Error:
"CoroutineType[Any, Any, AsyncGenerator[ChunkBatch, None]]" is not iterable
Fixed Code:
Fix Explanation: Removed explicit
AsyncGenerator
return type annotations and addedyield
statement to make abstract method a proper async generator. Assumption: Type checker can infer correct async generator types. Alternative: Could usetyping.AsyncGenerator
explicitly, but implicit typing is cleaner for async generators.3. Union Type Handling for GenerationResponse
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 1105-1113
Original Error:
Operator "+" not supported for types "str" and "str | List[dict[Unknown, Unknown]]"
Fixed Code:
Fix Explanation: Added type guards using
isinstance()
to handleUnion[str, List[dict]]
response type safely. Assumption: Response type correlates with prompt vs message mode. Alternative: Could cast types, but runtime checks are safer.🟢 Low Risk (15 fix categories)
1. Optional Member Access with None Checks
Files:
nemoguardrails/rails/llm/config.py
,nemoguardrails/rails/llm/llmrails.py
Lines: Multiple locations (1671, 1263, 1314, etc.)
Original Error:
"enabled" is not a known attribute of "None"
Fixed Code:
Fix Explanation: Added None checks before accessing attributes on optional objects. Assumption: None values should be treated as False/disabled. Alternative: Could use
getattr()
with defaults, but explicit checks are clearer.2. Context Variable Type Annotations
Files:
nemoguardrails/context.py
Lines: 24-40
Original Error:
Argument of type "ExplainInfo" cannot be assigned to parameter "value" of type "None"
Fixed Code:
Fix Explanation: Added explicit type annotations to context variables using forward references. Assumption: All context variables can hold None values. Alternative: Could use
Any
type, but explicit typing is better for type safety.3. Dynamic Attribute Access with getattr()
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: Multiple locations for options.log, options.dict, etc.
Original Error:
Cannot access attribute "dict" for class "dict[Unknown, Unknown]"
Fixed Code:
Fix Explanation: Used
getattr()
with defaults to safely access attributes on union types (dict vs typed objects). Assumption: Dict and object interfaces can be unified through getattr pattern. Alternative: Could use hasattr checks, but getattr with defaults is more concise.4. Optional Parameter None Handling
Files:
nemoguardrails/rails/llm/config.py
,nemoguardrails/rails/llm/llmrails.py
Lines: 1051, 206, 214-217, 926
Original Error:
Argument of type "Path | None" cannot be assigned to parameter "railsignore_path"
Fixed Code:
Fix Explanation: Added conditional checks or default values for optional parameters. Assumption: Empty collections are appropriate defaults. Alternative: Could modify function signatures, but preserving existing APIs is safer.
5. Type Annotation Corrections
Files:
nemoguardrails/utils.py
,nemoguardrails/rails/llm/llmrails.py
Lines: 378, 1469, 1479, 1484
Original Error:
Expected class but received "(obj: object, /) -> TypeIs[(...) -> object]"
Fixed Code:
Fix Explanation: Corrected type annotations from
callable
toCallable
and fixed parameter types. Assumption: Modern typing conventions should be used. Alternative: Could keep old-style annotations, but modern typing is preferred.6. Variable Scope and Binding Issues
Files:
nemoguardrails/rails/llm/config.py
Lines: 1168-1180
Original Error:
"content" is possibly unbound
Fixed Code:
Fix Explanation: Moved variable assignment outside try block to ensure it's bound before exception handler. Assumption: Variable needed in exception handler should be bound before try block. Alternative: Could initialize with default value, but moving assignment is cleaner.
7. Dictionary vs Object Type Flexibility
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 1032, 1024
Original Error:
Argument of type "list[Unknown]" cannot be assigned to parameter "value" of type "str"
Fixed Code:
Fix Explanation: Used flexible
dict
type annotation instead of inferred typed dict. Assumption: Message dictionaries can have mixed value types. Alternative: Could define proper TypedDict, but generic dict is more flexible.8. List Concatenation with Optional Types
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 899, 1045
Original Error:
Operator "+" not supported for types "list[dict[str, Unknown]]" and "List[dict[Unknown, Unknown]] | None"
Fixed Code:
Fix Explanation: Added default empty list for None values in concatenation operations. Assumption: Empty list is appropriate default for None message lists. Alternative: Could use conditional logic, but inline defaults are cleaner.
9. Module Import Safety Checks
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 214-217
Original Error:
"loader" is not a known attribute of "None"
Fixed Code:
Fix Explanation: Added None checks for spec and spec.loader before use. Assumption: Failed imports should be silently skipped. Alternative: Could raise exceptions, but graceful degradation is safer.
10. Streaming Handler Type Compatibility
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 953, 1068, 1296
Original Error:
Argument of type "object" cannot be assigned to parameter "chunk"
Fixed Code:
Fix Explanation: Added type ignore comments for END_OF_STREAM object compatibility. Assumption: END_OF_STREAM is intentionally designed to work with push_chunk. Alternative: Could modify END_OF_STREAM type, but type ignore is safer for existing code.
11. Function Return Type Enhancement
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 1529-1531
Original Error:
Type "ExplainInfo | None" is not assignable to return type "ExplainInfo"
Fixed Code:
Fix Explanation: Added lazy initialization to ensure non-None return value. Assumption: _ensure_explain_info() always returns valid object. Alternative: Could make return type optional, but ensuring object exists is better API.
12. Optional Object Attribute Access
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 1231-1238
Original Error:
"internal_events" is not a known attribute of "None"
Fixed Code:
Fix Explanation: Added None check for res.log before setting attributes. Assumption: Log object should exist before setting attributes. Alternative: Could create log object if None, but checking existing object is safer.
13. Configuration Streaming Safety
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 1649-1650
Original Error:
Argument of type "OutputRailsStreamingConfig | None" cannot be assigned
Fixed Code:
Fix Explanation: Added explicit None check with descriptive error. Assumption: Streaming operations require valid config. Alternative: Could provide default config, but failing fast is clearer.
14. Dynamic Result Attribute Access
Files:
nemoguardrails/rails/llm/llmrails.py
Lines: 1725-1728
Original Error:
Cannot access attribute "events" for class "str"
Fixed Code:
Fix Explanation: Used getattr to safely access events attribute on union type result. Assumption: Events attribute may not exist on all result types. Alternative: Could use hasattr check, but getattr with default is more concise.
15. Pydantic Field Corrections
Files:
nemoguardrails/rails/llm/options.py
Lines: Multiple locations for Field definitions and None handling
Original Error: Various Pydantic field and None operation errors
Fixed Code:
Fix Explanation: Fixed typos in Field descriptions and added None checks for arithmetic operations. Assumption: None values should be treated as zero or False in calculations. Alternative: Could use default values, but explicit None checks are clearer.
Summary
Test Plan
Type-checking
Unit-tests
Local CLI check
Related Issue(s)
Top-level PR to merge into before develop-branch merge: #1367
Checklist