-
Notifications
You must be signed in to change notification settings - Fork 106
Fix more type errors #977
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?
Fix more type errors #977
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.
Nothing blocking
temporalio/common.py
Outdated
@@ -729,7 +729,7 @@ def with_additional_attributes( | |||
... | |||
|
|||
|
|||
class MetricCounter(MetricCommon): | |||
class MetricCounter(MetricCommon, ABC): |
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.
This is interesting. Is re-inheriting ABC at every level a general Python preference or just a preference of a certain tool?
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.
Pyright was telling us to do it since they were effectively still ABCs
https://docs.basedpyright.com/v1.31.0/configuration/config-files/#reportImplicitAbstractClass
We can disable any we don't like, but I was thinking that we should err on the side of going with them.

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.
Hm, actually that's basedpyright-specific. I'm going to revert it.
**_WorkflowExternFunctions( | ||
__temporal_opentelemetry_completed_span=self._completed_workflow_span, | ||
) | ||
{ | ||
"__temporal_opentelemetry_completed_span": self._completed_workflow_span, | ||
} |
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.
What is the harm in using the constructor form of the TypedDict here instead of the dict literal with string literals?
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 couldn't see how to make it work with pyright (and your comment already indicated mypy probelems)
$ uv run pyright
/Users/dan/src/temporalio/sdk-python/temporalio/contrib/opentelemetry.py
/Users/dan/src/temporalio/sdk-python/temporalio/contrib/opentelemetry.py:130:15 - error: Argument of type "object" cannot be assigned to parameter "kwargs" of type "(...) -> Unknown" in function "update"
Type "object" is not assignable to type "(...) -> Unknown" (reportArgumentType)
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.
Yeah, unfortunate that we can't use that nice constructor-like kwarg approach afforded to us by typed dicts
b9e35ed
to
382ee2c
Compare
Paramaterize OpenAI agents with context type in test
382ee2c
to
621525c
Compare
Miscellaneous type error fixes, making progress towards fixing them all, opening PR for this subset since this is how many got done right now and we don't want them to pick up conflicts.
Reviewers: the commit named "interesting" contains slightly more interesting fixes.