-
Notifications
You must be signed in to change notification settings - Fork 549
chore(types): Type-clean tracing (10 errors) #1388
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
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.
Minor comments only on my side.
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.
Thanks for fixing this @tgasser-nv 👍🏻
Converting to draft while I rebase on the latest changes to develop. |
e939b0a
to
b5dd426
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Rebased on top of develop and re-ran all the tests / pre-commit checks |
Description
Type-cleaned the nemoguardrails/tracing directory, added this to the pre-commits so this can't regress over time.
Type Fixes Summary for
nemoguardrails/tracing/
DirectoryHere is a summary of the type-safety fixes in the provided diff, categorized by risk level.
High Risk
nemoguardrails/tracing/span_extractors.py
AttributeError
error
attribute on anaction
object which may not exist, leading to a potentialAttributeError
. The fix replaces this withNone
, which is a safe default. This is considered high risk because it fundamentally changes the error handling logic.None
for error-related fields.error
attribute before accessing it. However, the implemented fix is more explicit and safer.Medium Risk
File:
nemoguardrails/tracing/span_extractors.py
Line: 62
Error: Type variable mismatch.
Fix:
Analysis: The original code had a type hint of
List[SpanLegacy]
but the function could also returnSpanOpentelemetry
objects. The fix correctly changes the type hint toList[Union[SpanLegacy, SpanOpentelemetry]]
. This is medium risk as it could affect downstream type checking.Assumptions: Assumes that downstream consumers of this function can handle a list containing both
SpanLegacy
andSpanOpentelemetry
objects.Alternatives: Another approach could be to have the function always return a single type, but this would require more significant refactoring.
File:
nemoguardrails/tracing/tracer.py
Line: 69
Error: Potential
None
value.Fix:
Analysis: This fix adds a check to ensure
generation_log
is notNone
before it is used. This is a good defensive programming practice and is considered medium risk because it introduces a new failure mode (an exception) where previously the code might have failed silently or with a less clear error.Assumptions: This change assumes that it is better to fail fast with an explicit error.
Alternatives: The original code could have been modified to handle the
None
case gracefully, but raising an exception is a reasonable choice.Low Risk
File:
nemoguardrails/tracing/interaction_types.py
Line: 81
Error: Potential
None
value.Fix:
Analysis: The original code assigned
generation_log.internal_events
toevents
, which could beNone
. The fix ensures that ifinternal_events
isNone
, an empty list is used instead. This is a low-risk and safe change.Assumptions: The assumption is that an empty list is a valid and expected value for
events
.Alternatives: An
if
statement could have been used, but theor []
is more concise and Pythonic.Test Plan
Type-checking
$ poetry run pre-commit run --all-files check yaml...............................................................Passed fix end of files.........................................................Passed trim trailing whitespace.................................................Passed isort (python)...........................................................Passed black....................................................................Passed Insert license in comments...............................................Passed pyright..................................................................Passed
Unit-tests
Local CLI check
Checklist