Skip to content

Make client.py typecheck under pyright #628

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 6, 2024
Merged

Make client.py typecheck under pyright #628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fffb31a
Make client.py typecheck under pyright
dandavison Aug 26, 2024
aa4ab92
Create a cluster of type ignore annotations
dandavison Aug 28, 2024
5efb27d
Fix accidental dataclass / IntEnum chimera
dandavison Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ exclude = [
"temporalio/bridge/proto",
"tests/worker/workflow_sandbox/testmodules/proto",
"temporalio/bridge/worker.py",
"temporalio/client.py",
"temporalio/contrib/opentelemetry.py",
"temporalio/converter.py",
"temporalio/testing/_workflow.py",
Expand Down
21 changes: 10 additions & 11 deletions temporalio/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import google.protobuf.duration_pb2
import google.protobuf.json_format
import google.protobuf.timestamp_pb2
from typing_extensions import Concatenate, TypedDict
from typing_extensions import Concatenate, Required, TypedDict

import temporalio.api.common.v1
import temporalio.api.enums.v1
Expand Down Expand Up @@ -1112,12 +1112,12 @@ async def get_worker_task_reachability(
class ClientConfig(TypedDict, total=False):
"""TypedDict of config originally passed to :py:meth:`Client`."""

service_client: temporalio.service.ServiceClient
namespace: str
data_converter: temporalio.converter.DataConverter
interceptors: Sequence[Interceptor]
default_workflow_query_reject_condition: Optional[
temporalio.common.QueryRejectCondition
service_client: Required[temporalio.service.ServiceClient]
namespace: Required[str]
Comment on lines +1115 to +1116
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, weird, you think Required would be the default...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there's a fair amount of reading material on the topic for anyone so-inclined.
https://peps.python.org/pep-0655/
https://typing.readthedocs.io/en/latest/spec/typeddict.html#required-notrequired

Copy link
Member

@cretz cretz Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmm, I am trying to remember why I did total=False here. Technically all of these config values are optional if you're using it as a typesafe way to provide parameters to Client.__init__. I wonder if I expected someone to do something like:

extra: client.ClientConfig = { "interceptors": [MyInterceptor()] }

# ...

new_client = Client(my_service_client, **extra)

This PR would break that use case. Also it means that if we ever add fields to __init__ we cannot make them required here if we want to be compatible with people that may be using this type. If we are ok breaking this use case, then I wonder if we should just remove total=False. I can't think of a reason that total=False should be there if every field is Required, can anyone else? Or if we want to allow these to be optional, no need for these new annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm not sure I should have merged without resolving this conversation. May need to revisit this. E.g. in features repo we attempt to do

register_feature(
    workflows=[Workflow],
    check_result=check_result,
    start=start,
    additional_client_config=ClientConfig(interceptors=[MyClientInterceptor()]),
)

https://github.com/temporalio/features/blob/bump-sdk-version/bin/main/update/client_interceptor/feature.py#L66

which isn't passing type checks.

data_converter: Required[temporalio.converter.DataConverter]
interceptors: Required[Sequence[Interceptor]]
default_workflow_query_reject_condition: Required[
Optional[temporalio.common.QueryRejectCondition]
]


Expand Down Expand Up @@ -1797,7 +1797,7 @@ async def execute_update(
MultiParamSpec, LocalReturnType
],
*,
args: MultiParamSpec.args,
args: MultiParamSpec.args, # pyright: ignore
id: Optional[str] = None,
rpc_metadata: Mapping[str, str] = {},
rpc_timeout: Optional[timedelta] = None,
Expand Down Expand Up @@ -1906,7 +1906,7 @@ async def start_update(
MultiParamSpec, LocalReturnType
],
*,
args: MultiParamSpec.args,
args: MultiParamSpec.args, # pyright: ignore
wait_for_stage: WorkflowUpdateStage,
id: Optional[str] = None,
rpc_metadata: Mapping[str, str] = {},
Expand Down Expand Up @@ -3233,7 +3233,7 @@ class ScheduleActionStartWorkflow(ScheduleAction):
headers: Optional[Mapping[str, temporalio.api.common.v1.Payload]]

@staticmethod
def _from_proto(
def _from_proto( # pyright: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmm, I wonder if this is the same thing we ignore in the system-agnostic way below and pyright just cares about the line the declaration started instead of the param.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's right. It's a bit silly having a cluster of type-ignore annotations for different type-checkers but I played around with a few variations and this is what passed.

info: temporalio.api.workflow.v1.NewWorkflowExecutionInfo, # type: ignore[override]
) -> ScheduleActionStartWorkflow:
return ScheduleActionStartWorkflow("<unset>", raw_info=info)
Expand Down Expand Up @@ -3482,7 +3482,6 @@ async def _to_proto(
return action


@dataclass
class ScheduleOverlapPolicy(IntEnum):
"""Controls what happens when a workflow would be started by a schedule but
one is already running.
Expand Down
3 changes: 0 additions & 3 deletions temporalio/worker/_workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,15 @@
import os
import sys
from datetime import timezone
from threading import get_ident
from types import TracebackType
from typing import (
Callable,
Dict,
List,
Literal,
MutableMapping,
Optional,
Sequence,
Set,
Tuple,
Type,
)

Expand Down
4 changes: 2 additions & 2 deletions temporalio/worker/_workflow_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ async def run_update() -> None:
else:
self._current_activation_error = err
return
except BaseException as err:
except BaseException:
if self._deleting:
if LOG_IGNORE_DURING_DELETE:
logger.debug(
Expand Down Expand Up @@ -883,7 +883,7 @@ async def run_workflow(input: ExecuteWorkflowInput) -> None:
)
self._primary_task = self.create_task(
self._run_top_level_workflow_function(run_workflow(input)),
name=f"run",
name="run",
)

def _apply_update_random_seed(
Expand Down
6 changes: 3 additions & 3 deletions tests/helpers/external_coroutine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,17 @@
File used in conjunction with external_stack_trace.py to test filenames in multi-file workflows.
"""

from asyncio import sleep
from typing import List

from temporalio import workflow


async def never_completing_coroutine(status) -> None:
async def never_completing_coroutine(status: List[str]) -> None:
status[0] = "waiting" # external coroutine test
await workflow.wait_condition(lambda: False)


async def wait_on_timer(status) -> None:
async def wait_on_timer(status: List[str]) -> None:
status[0] = "waiting" # multifile test
print("Coroutine executed, waiting.")
await workflow.wait_condition(lambda: False)
Loading