Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 18 additions & 12 deletions taskiq/decor.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import inspect
import sys
from collections.abc import Callable, Coroutine
from copy import copy
Expand Down Expand Up @@ -75,18 +76,23 @@ def __init__(
# it back to the module where it was defined.
# This way ProcessPoolExecutor will be able to import
# the function by it's name and verify its correctness.
new_name = f"{original_func.__name__}__taskiq_original"
self.original_func.__name__ = new_name
if hasattr(self.original_func, "__qualname__"):
original_qualname = self.original_func.__qualname__.rsplit(".")
original_qualname[-1] = new_name
new_qualname = ".".join(original_qualname)
self.original_func.__qualname__ = new_qualname
setattr(
sys.modules[original_func.__module__],
new_name,
original_func,
)
#
# Bound methods are skipped: their `__name__` is not
# assignable, and renaming via `__func__` would mutate
# the class-level function and break pickle lookup.
if inspect.isfunction(original_func):
new_name = f"{original_func.__name__}__taskiq_original"
self.original_func.__name__ = new_name
if hasattr(self.original_func, "__qualname__"):
original_qualname = self.original_func.__qualname__.rsplit(".")
original_qualname[-1] = new_name
new_qualname = ".".join(original_qualname)
self.original_func.__qualname__ = new_qualname
setattr(
sys.modules[original_func.__module__],
new_name,
original_func,
)

# Docs for this method are omitted in order to help
# your IDE resolve correct docs for it.
Expand Down
59 changes: 59 additions & 0 deletions tests/abc/test_broker.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
from collections.abc import AsyncGenerator
from concurrent.futures import ProcessPoolExecutor
from copy import copy
from types import MethodType

import pytest

from taskiq import InMemoryBroker
from taskiq.abc.broker import AsyncBroker
from taskiq.decor import AsyncTaskiqDecoratedTask
from taskiq.events import TaskiqEvents
Expand Down Expand Up @@ -30,6 +33,15 @@ async def listen(self) -> AsyncGenerator[BrokerMessage, None]: # type: ignore
"""


_process_pool_broker = _TestBroker()


@_process_pool_broker.task(task_name="process_pool_sync_add")
def process_pool_sync_add(value: int) -> int:
"""Module-level sync task used by ProcessPoolExecutor regression test."""
return value + 1


def test_decorator_success() -> None:
"""Test that decoration without parameters works."""
tbrok = _TestBroker()
Expand Down Expand Up @@ -82,6 +94,53 @@ async def test_task() -> None: ...
assert test_task.labels == old_labels


def test_register_task_accepts_bound_method() -> None:
"""Bound methods must register without mutating method.__name__."""
broker = _TestBroker()

class Counter:
def increment(self, value: int) -> int:
return value + 1

instance = Counter()
task = broker.register_task(instance.increment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a problem with this approach and it should not be allowed to do so.

You register a task as a function which is bound to a particular instance of the class. The problem is that there is a hidden state in the class.

For example, consider this:

class Sateful:
    def __init__(self, inc: int) -> None:
		self.inc = inc
	
	def my_task(self, value: int) -> int:
		return self.value + self.inc

instance = Counter()
task = broker.register_task(instance.my_task)

There are few questions to be answered:

  • How do we make sure that the state of the class matches the state of the task?
  • What to do in case if it doesn't?
  • It it's an instance, what should be the name of such function?
  • If there are multiple instances of the same class with different state, how do we handle it?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hello! Thanks for the review - these are fair design questions!

Quick disclaimer: this is my first open-source contribution in a long time, so I may be missing some project context or conventions. Happy to adjust based on your guidance.

I agree that registering a live bound method is problematic: it captures hidden instance state that is not part of the task message. Taskiq’s model is closer to "lookup callable by name on the worker", so it cannot guarantee that client-side self matches worker-side self.

How do we make sure class state matches the task?

With bound methods - we don’t, at least not automatically. The app would have to register the same object on the worker during startup/import. That is fragile for distributed workers.

What to do if it doesn’t?

Same as today for a missing/wrong registration: task not found, or the wrong callable runs. I don’t think core should serialize self into the message or try to validate instance identity across processes.

What should the name be for an instance method?

Current default module:__name__ is fine for functions. For instance methods it becomes ambiguous if several instances share the same method name. Explicit task_name=... would be required, but that still doesn’t solve state sync.

Multiple instances with different state?

They would collide on the same default name, and last registration wins. Auto-namespacing by id(instance) would be unstable across processes, so I wouldn’t go that way.

So I don’t think “officially support register_task(instance.method)” is the right long-term answer for the project

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Maybe we can do instead, if OOP-style tasks are desirable, is class-based tasks or maybe some sort of factory-based tasks:

  • register a class, not a live instance
  • worker instantiates it per execution, like MyTask(broker, message).run(*args, **kwargs)
  • instance state is created on the worker (config, DI, labels), not captured from the client
  • task name is stable: module:ClassName or explicit task_name;
  • multiple logical variants are different classes / different names / different kwargs, not different live objects

That removes the hidden-state problem and fits the existing registry model much better than bound methods.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I would like to get some answers from you or other people with project context.

  • For bound methods short-term, do you prefer: reject them with a clear error or keep no-crash registration only as unsupported behavior?
  • Are you open to class-based tasks as a follow-up feature?
  • If yes to class-based tasks, what should the minimal API look like to you: like @broker.task on a class with run(...), or explicit MyTask.register(broker), or idk....
  • Should task instance state come mainly from constructor args / factory or DI or some meta or ...

Happy to reshape this PR once you say which direction you want)


assert isinstance(task, AsyncTaskiqDecoratedTask)
assert isinstance(task.original_func, MethodType)
assert task.original_func.__self__ is instance
assert task.task_name == f"{instance.increment.__module__}:increment"
assert broker.find_task(task.task_name) is task


@pytest.mark.anyio
async def test_bound_method_task_executes_with_self() -> None:
"""Registered bound method must keep instance state across execution."""
broker = InMemoryBroker(await_inplace=True)

class Counter:
def __init__(self) -> None:
self.total = 0

def add(self, value: int) -> int:
self.total += value
return self.total

instance = Counter()
task = broker.register_task(instance.add, task_name="bound.add")

kicked = await task.kiq(5)
result = await kicked.wait_result()
assert result.return_value == 5
assert instance.total == 5


def test_process_pool_runs_module_level_decorated_sync_function() -> None:
"""ProcessPoolExecutor must be able to run renamed original sync functions."""
with ProcessPoolExecutor(max_workers=1) as executor:
future = executor.submit(process_pool_sync_add.original_func, 41)
assert future.result(timeout=5) == 42


@pytest.mark.anyio
@pytest.mark.parametrize(
("is_worker_process", "startup", "shutdown"),
Expand Down
Loading