Skip to content

Minor cleanup #861

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 1 commit into from
May 19, 2025
Merged

Minor cleanup #861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions temporalio/worker/_interceptor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,23 +388,23 @@ async def signal_external_workflow(

def start_activity(
self, input: StartActivityInput
) -> temporalio.workflow.ActivityHandle:
) -> temporalio.workflow.ActivityHandle[Any]:
"""Called for every :py:func:`temporalio.workflow.start_activity` and
:py:func:`temporalio.workflow.execute_activity` call.
"""
return self.next.start_activity(input)

async def start_child_workflow(
self, input: StartChildWorkflowInput
) -> temporalio.workflow.ChildWorkflowHandle:
) -> temporalio.workflow.ChildWorkflowHandle[Any, Any]:
"""Called for every :py:func:`temporalio.workflow.start_child_workflow`
and :py:func:`temporalio.workflow.execute_child_workflow` call.
"""
return await self.next.start_child_workflow(input)

def start_local_activity(
self, input: StartLocalActivityInput
) -> temporalio.workflow.ActivityHandle:
) -> temporalio.workflow.ActivityHandle[Any]:
"""Called for every :py:func:`temporalio.workflow.start_local_activity`
and :py:func:`temporalio.workflow.execute_local_activity` call.
"""
Expand Down
16 changes: 5 additions & 11 deletions temporalio/worker/_workflow_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1590,12 +1590,10 @@ def _outbound_schedule_activity(
"Activity must have start_to_close_timeout or schedule_to_close_timeout"
)

handle: Optional[_ActivityHandle] = None
handle: _ActivityHandle
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, can the static analyzer know that this is set before used? Or do the Python static analyzers not check whether a value is set before use?

Copy link
Contributor Author

@dandavison dandavison May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

They can't detect all situations where it is read before being set. So in that case it would be a run-time error, similar to the AssertionError that would have been thrown by the code prior to this commit. Since the code algorithmically guarantees that these run-time errors do not in fact occur, IMO it is more idiomatic and cleaner not to set an initial value that is never used.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it is more idiomatic and cleaner not to set an initial value that is never used

Never used now but you have no protection anymore. I think it's safer to only ever capture variables in a nested function that have been initialized, even if you promise to re-initialize them again later. And nonlocal and assert signal this intent.

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 it's safer

How is it safer -- it's a run-time error either way, either an AssertionError or a NameError / some sort of unbound variable error. Either way, the problem will be simple to track down.

Copy link
Member

@cretz cretz May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's safer from a general logic POV to have no possibility of uninitialized variable access (in any programming language). And it's also clearer from a reader POV that you intended for this situation via nonlocal/assert, which helps safety/comprehension. Python is not smart enough to enforce accessing an only initialized variables like some statically typed languages do, hence the help. Sure it may not matter much here, definitely not enough to change from what it was, but I'm not pushing back on you changing it.


# Function that runs in the handle
async def run_activity() -> Any:
nonlocal handle
assert handle
while True:
# Mark it as started each loop because backoff could cause it to
# be marked as unstarted
Expand Down Expand Up @@ -1662,12 +1660,10 @@ async def _outbound_signal_external_workflow(
async def _outbound_start_child_workflow(
self, input: StartChildWorkflowInput
) -> _ChildWorkflowHandle:
handle: Optional[_ChildWorkflowHandle] = None
handle: _ChildWorkflowHandle

# Common code for handling cancel for start and run
def apply_child_cancel_error() -> None:
nonlocal handle
assert handle
# Send a cancel request to the child
cancel_command = self._add_command()
handle._apply_cancel_command(cancel_command)
Expand All @@ -1685,9 +1681,7 @@ def apply_child_cancel_error() -> None:

# Function that runs in the handle
async def run_child() -> Any:
nonlocal handle
while True:
assert handle
try:
# We have to shield because we don't want the future itself
# to be cancelled
Expand Down Expand Up @@ -2438,17 +2432,17 @@ async def signal_external_workflow(

def start_activity(
self, input: StartActivityInput
) -> temporalio.workflow.ActivityHandle:
) -> temporalio.workflow.ActivityHandle[Any]:
return self._instance._outbound_schedule_activity(input)

async def start_child_workflow(
self, input: StartChildWorkflowInput
) -> temporalio.workflow.ChildWorkflowHandle:
) -> temporalio.workflow.ChildWorkflowHandle[Any, Any]:
return await self._instance._outbound_start_child_workflow(input)

def start_local_activity(
self, input: StartLocalActivityInput
) -> temporalio.workflow.ActivityHandle:
) -> temporalio.workflow.ActivityHandle[Any]:
return self._instance._outbound_schedule_activity(input)


Expand Down
2 changes: 1 addition & 1 deletion tests/helpers/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import uuid
from contextlib import closing
from datetime import timedelta
from typing import Any, Awaitable, Callable, Optional, Sequence, Type, TypeVar, Union
from typing import Any, Awaitable, Callable, Optional, Sequence, Type, TypeVar

from temporalio.api.common.v1 import WorkflowExecution
from temporalio.api.enums.v1 import IndexedValueType
Expand Down
Loading