fix: allow registering bound methods as tasks - #653
Conversation
Skip ProcessPoolExecutor name rewriting for non-functions so broker.register_task works with bound methods again (taskiq-python#436).
| return value + 1 | ||
|
|
||
| instance = Counter() | ||
| task = broker.register_task(instance.increment) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
selfinto 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. Explicittask_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
There was a problem hiding this comment.
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:ClassNameor 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.
There was a problem hiding this comment.
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.taskon a class withrun(...), 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)
Summary
__name__/__qualname__rewriting unlessoriginal_funcis a plain Python function (inspect.isfunction)broker.register_task(instance.method, ...)for bound methods (No longer able to register methods in TaskIQ 0.11.16 #436)self, and ProcessPoolExecutor pickle/import of module-level sync tasksFixes #436