-
Notifications
You must be signed in to change notification settings - Fork 572
chore(types): Type-clean colang/ (111 errors) #1381
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
base: develop
Are you sure you want to change the base?
Conversation
|
Tagging @cparisien, @Pouyanpi , @trebedea for review |
Documentation preview |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
f5e281f to
489b2a3
Compare
b4a4f85 to
f507ea0
Compare
|
Converting to draft while I rebase on the latest changes to |
f507ea0 to
76caade
Compare
|
Refreshed this PR based on the latest |
|
@tgasser-nv I created a dummy commit and can see around 63 pyright errors when pre-commits are run. Would you please have a look? |
647cc32 to
dbeba8b
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.
Greptile Overview
Greptile Summary
This PR systematically eliminates 111 Pyright type-checking errors across the nemoguardrails/colang/ directory by adding defensive null-checks, type guards, and explicit type annotations. The changes prevent potential runtime TypeError, AttributeError, and KeyError exceptions by validating that variables are not None before dereferencing, providing default values for optional parameters, and wrapping non-dict action results into dictionaries where expected. The modifications span both Colang v1.0 and v2.x parsers, runtimes, and AST processors, following a defensive programming pattern that silently skips operations when values are missing rather than failing fast. This aligns with the broader Guardrails architecture where robustness in parsing and flow execution is prioritized over strict validation.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
pyproject.toml |
5/5 | Adds nemoguardrails/colang/** to Pyright include list to enable type-checking for the module |
nemoguardrails/logging/processing_log.py |
5/5 | Adds type annotation to processing_log_var context variable for improved static analysis |
nemoguardrails/actions/llm/generation.py |
5/5 | Removes explicit -> None return type hint to fix type-checker complaints about implicit returns |
nemoguardrails/colang/v2_x/lang/colang_ast.py |
5/5 | Adds __hash__ method to Element class to satisfy Python's hashable protocol |
nemoguardrails/colang/v2_x/runtime/serialization.py |
4/5 | Adds type guard after deserialization to ensure State instance before accessing attributes |
nemoguardrails/colang/v1_0/runtime/sliding.py |
5/5 | Wraps imports in TYPE_CHECKING block to resolve circular dependencies |
nemoguardrails/colang/v2_x/runtime/flows.py |
5/5 | Uses TYPE_CHECKING guard for RailsConfig import to prevent circular dependency |
nemoguardrails/colang/v1_0/lang/comd_parser.py |
5/5 | Adds null-check for sym variable before processing symbol mappings |
nemoguardrails/colang/runtime.py |
4/5 | Adds null-check for config.imported_paths and uses getattr() for conditional method registration |
nemoguardrails/colang/v2_x/lang/parser.py |
4/5 | Corrects return type annotation and adds null-check for import_el.package |
nemoguardrails/colang/v2_x/lang/transformer.py |
5/5 | Renames variable to avoid shadowing and broadens return type to Any |
nemoguardrails/colang/v2_x/runtime/eval.py |
5/5 | Fixes lambda closure bugs and adds type-cast for SimplifyFormatter().format() |
nemoguardrails/colang/v1_0/lang/colang_parser.py |
3/5 | Adds extensive null-checks and type annotations throughout parser logic |
nemoguardrails/colang/v1_0/lang/utils.py |
3/5 | Adds defensive null-checks for string variables and introduces unused multiline_indentation |
nemoguardrails/colang/v1_0/runtime/flows.py |
3.5/5 | Adds null-checks for slide() return values but has return-type inconsistency in _slide_with_subflows() |
nemoguardrails/colang/v1_0/runtime/runtime.py |
4/5 | Adds guards for None values and wraps non-dict action results silently |
nemoguardrails/colang/v2_x/lang/expansion.py |
4/5 | Adds compound type-safety guards for element._source.line and spec element validation |
nemoguardrails/colang/v2_x/lang/utils.py |
5/5 | Adds critical type guard to prevent TypeError when calling asdict() on dataclass classes |
nemoguardrails/colang/v2_x/runtime/statemachine.py |
4/5 | Comprehensive null-checks across state machine runtime with repeated defensive patterns |
nemoguardrails/colang/v1_0/lang/coyml_parser.py |
4/5 | Adds defensive checks for dictionary keys and converts jump offsets to strings |
nemoguardrails/colang/v2_x/runtime/runtime.py |
4/5 | Adds conditional checks for langchain attributes and filters Event objects from history |
Confidence score: 3.5/5
- This PR improves robustness but introduces several silent failure modes where exceptions or missing values are ignored rather than raising errors, which could mask bugs
- Score reflects that while the type-safety improvements reduce immediate crash risk, several changes have medium risk:
_slide_with_subflows()always returnsNonedespite declaringOptional[int]return type, unusedmultiline_indentationvariable in utils.py, duplicateRailsConfigimport in flows.py, silent wrapping of action results without logging, and repeated defensive patterns that could benefit from refactoring - Pay close attention to
nemoguardrails/colang/v1_0/runtime/flows.py(return type mismatch),nemoguardrails/colang/v1_0/lang/utils.py(dead code), andnemoguardrails/colang/v1_0/runtime/runtime.py(silent error suppression)
Additional Comments (1)
-
nemoguardrails/colang/v1_0/lang/coyml_parser.py, line 437-450 (link)style: Converting
_next_elsefrom int to str and then back to int (line 450) is unnecessary. Keep it as int throughout or document why string representation is required by the runtime.
21 files reviewed, 19 comments
| if not isinstance(state, State): | ||
| raise ValueError("Decoded object is not a State instance") |
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.
logic: Check that decode_from_dict returns a dict with expected structure before casting to State, since decode_from_dict can return primitives or other types. Should we verify the decoded data has the expected structure (e.g., flow_states attribute) before the isinstance check?
| stripped_tokens = get_stripped_tokens(split_max(flow_name, "=", 1)) | ||
| return_vars, flow_name = stripped_tokens[0], stripped_tokens[1] |
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.
logic: When split_max returns only 1 token, stripped_tokens[1] will cause IndexError
| current_comment = None | ||
| multiline_string = False | ||
| current_string = None | ||
| multiline_indentation = 0 |
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.
logic: multiline_indentation is initialized but never written to after this point. The value set on line 120 is never used because it's only set inside the multiline_string block that never reads this variable.
| # Ensure element is valid for SpecOp | ||
| if isinstance(element, (dict, Spec)): | ||
| spec_element: Union[dict, Spec] = element | ||
| else: | ||
| spec_element = {} |
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.
style: Wrapping with an empty dict {} as fallback could hide programming errors. If element is not a valid dict or Spec, the error might surface later in unexpected ways rather than failing fast here. Consider logging a warning or using a more defensive default. Should invalid elements be logged or raise an error instead of silently defaulting to {}?
f43e73e to
84484f5
Compare
Greptile OverviewGreptile SummaryThis PR comprehensively type-cleans the Important Files Changed
|
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.
21 files reviewed, no comments
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.
21 files reviewed, no comments
Description
Summary
Type-cleaning nemoguardrails/colang directory.
Test Plan
Pre-commit (includes type-checking)
Unit-tests
Interactive chat (Colang 1)
Interactive chat (Colang 2)
Checklist