-
Notifications
You must be signed in to change notification settings - Fork 49
Chore/type checking types 01 #105
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: main
Are you sure you want to change the base?
Conversation
…(12 remaining) Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Chipping off the remaining work in #30 |
@@ -183,7 +184,7 @@ class ChatCompletion(BaseModel): | |||
object: Optional[str] = None | |||
usage: dict | |||
|
|||
def get_message(self) -> Optional[str]: | |||
def get_message(self) -> Optional[Dict[str, Any]]: |
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.
I do not think this is correct because getting Content
is just a plain string right?
def get_content(self) -> Optional[str]:
"""
Retrieve the content from the first choice's message.
"""
message = self.get_message()
return message.get("content") if message else None
A Response Example -> Choices -> 0 -> Message -> Content right?
{
"id": "chatcmpl-B9MBs8CjcvOU2jLn4n570S5qMJKcT",
"object": "chat.completion",
"created": 1741569952,
"model": "gpt-4.1-2025-04-14",
"choices": [
{
"index": 0,
"message": {
"role": "assistant",
"content": "Hello! How can I assist you today?",
"refusal": null,
"annotations": []
},
"logprobs": null,
"finish_reason": "stop"
}
],
"usage": {
"prompt_tokens": 19,
"completion_tokens": 10,
"total_tokens": 29,
"prompt_tokens_details": {
"cached_tokens": 0,
"audio_tokens": 0
},
"completion_tokens_details": {
"reasoning_tokens": 0,
"audio_tokens": 0,
"accepted_prediction_tokens": 0,
"rejected_prediction_tokens": 0
}
},
"service_tier": "default"
}
So having Opetional[str]
is the right thing here and no Optional[Dict[str, Any]]
right?
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.
@Cyb3rWard0g yes, the content
from get_content returns a plain string howeveer get_message returns from .model_dump()
which returns a Dict[str, Any]
:
def model_dump(
self,
*,
mode: Literal['json', 'python'] | str = 'python',
include: IncEx | None = None,
exclude: IncEx | None = None,
context: Any | None = None,
by_alias: bool | None = None,
exclude_unset: bool = False,
exclude_defaults: bool = False,
exclude_none: bool = False,
round_trip: bool = False,
warnings: bool | Literal['none', 'warn', 'error'] = True,
fallback: Callable[[Any], Any] | None = None,
serialize_as_any: bool = False,
) -> dict[str, Any]:
...
This then also ensures that when get_content
is fetched (which calls get_message
) the .get("content")
works since it's a dict (well, unless a None
was returned).
Hey @CasperGN! first off, Thank you very much for driving the mypy clean-up! Knocking the list down from 19 to 12 errors is an awesome first pass. 🚀 CommentsIf I understand correctly, all remaining complaints are the How we can resolve them
from typing import cast
# after coercion
if isinstance(configuration, dict):
configuration = OpenAIModelConfig(**configuration)
configuration = cast(
OpenAIModelConfig
| AzureOpenAIModelConfig
| HFHubModelConfig
| NVIDIAModelConfig,
configuration,
) Same pattern for if hasattr(value, "closed") and value.closed:
... or cast the tuple branch to Next steps?
Thanks again for jumping on this! almost there! |
Yes this is my understanding as well. @Cyb3rWard0g I concur with your preference of B. I don't think C is a good option and it's what is more or less already being done my just excluding errors from When looking over the other modules there's a lot of
Edit: Figured it out.
|
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
…ployment Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
Signed-off-by: Casper Guldbech Nielsen <[email protected]>
First iteration of fixing type errors reported by
mypy
fordapr_agents.types
:before
Now
Remaining
None.
@yaron2, @Cyb3rWard0g: How do you best want to deal with the
union-attr
error where some of theparameters.model
will not contain all possible configurations?EDIT:
Updated with the
cast
as per @Cyb3rWard0g . Now we're down to 0 errors. Updated above.