-
Notifications
You must be signed in to change notification settings - Fork 543
chore(types): Type-clean /cli (37 errors) #1380
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
spec: Spec = ( | ||
head_element_spec_op.spec |
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.
maybe
spec: Spec = (
head_element_spec_op.spec
if isinstance(head_element_spec_op.spec, SpecOp)
else Spec(**head_element_spec_op.spec) if isinstance(head_element_spec_op.spec, dict)
else None
)
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.
Yes, this is better. I think we should check for isinstance(head_element_spec_op.spec, Spec)
since the spec field in SpecOp is a Spec
rows.sort(key=lambda x: (-state.flow_configs[x[0]].loop_priority, x[0])) | ||
if not state: | ||
raise RuntimeError("No state available") | ||
flow_configs: Dict[str, FlowConfig] = state.flow_configs |
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.
nit: this check seems redundant
flow_configs: Dict[str, FlowConfig] = state.flow_configs
Will crash if state is None
I think the check if not state happens after state.flow_configs is already accessed on line 186 in the lambda sort key.
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.
Agreed, we know state
isn't None because of the check on line 123. I removed this one
nemoguardrails/cli/__init__.py
Outdated
from_version_literal: Literal["1.0", "2.0-alpha"] = ( | ||
"1.0" if from_version == ColangVersions.one else "2.0-alpha" | ||
) |
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.
from_version_literal: Literal["1.0", "2.0-alpha"] = from_version.value
Thank you @tgasser-nv . I also tested it against #1339 and all tests pass. Some of the changes might require further test cases, for example response type handling changes in chat.py:117-144. The existing tests only mock generate_async() to return simple dict responses, but don't test the new type-checking logic for: if type(response) == Tuple[Dict, Dict]:
elif type(response) == GenerationResponse: # untested
elif type(response) == Dict: # untested fallback logic |
I agree we need to add test-cases to cover these new options. Should we do this after #1339 is merged? |
Description
Clean up type errors in the nemoguards/cli directory
Test plan
Type-checking
Unit-tests
Chat local integration test
Related Issue(s)
Top-level PR for tracking : #1367
Checklist