-
Notifications
You must be signed in to change notification settings - Fork 54
Celery stubs improvements #210
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?
Conversation
Significantly expanded type stubs for the celery package: - Added many new modules: app/annotations, app/autoretry, app/backends, app/builtins, app/defaults, app/trace, apps/beat, apps/multi - Added backend stubs: arangodb, asynchronous, azureblockblob, cache, cassandra, consul, cosmosdbsql, couchbase, couchdb, dynamodb, elasticsearch, filesystem, gcs, mongodb, redis, rpc, s3 - Added bin stubs: amqp, base, beat, call, celery, control, events, graph, list, logtool, migrate, multi, purge, result, shell, upgrade, worker - Added concurrency stubs: asynpool, base, eventlet, gevent, prefork, solo, thread - Added worker stubs: components, consumer/*, control, heartbeat, loops, pidbox, request, state, strategy, worker - Added utils stubs: collections, debug, dispatch, functional, graph, imports, iso8601, log, nodenames, objects, serialization, term, text, threads, time, timer2 - Enhanced existing module type annotations with more complete signatures - Added @OverRide decorators for proper method override annotations
b0571fe to
e3460ff
Compare
| autofinalize: bool = True, | ||
| namespace: str | None = None, | ||
| strict_typing: bool = True, | ||
| broker_connection_retry: bool = ..., |
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.
Are all these arguments really missing? Or is this a mistake? I assume that these arguments are actually present in kwargs, and if so, I think it would be more useful to explicitly mark them as keyword-only using *,.
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.
Celery makes this kind of awkward, since it's not an explicit kwarg in the actual code. I guess keeping it makes more sense from a practical point.
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.
Please check the other places. I have seen it in at least one other class. And, by the way, perhaps it would make sense to mark them with '*,'?
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.
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.
You were totally right. I adjusted this code and checked all other similar cases. Hopefully I got everything.
….apply_async Restore explicit keyword arguments for better IDE autocomplete support: - Celery.__init__: Add config kwargs (imports, broker_*, result_*, task_*, worker_*) - Celery._task_from_fun: Add autoretry and task option kwargs - Signature.apply_async: Add options kwargs (task_id, producer, link, countdown, etc.) Use keyword-only syntax (*,) to satisfy stubtest while still providing typed kwargs for IDE support.
ed8bf79 to
60d818d
Compare
- Remove Proxy wrapper from current_app, current_task, default_app types
- send_task: Add explicit kwargs after * for better IDE autocomplete:
ignore_result, priority, queue, exchange, routing_key, exchange_type,
stamped_headers (all passed through **options at runtime)
- add_periodic_task: Fix kwargs type from tuple[()] to dict[str, Any] | None
(runtime uses empty tuple as falsy default, converted to {} internally)
Improve IDE autocomplete by adding explicit kwargs after * for all major public API methods that accept **options or **kwargs at runtime. canvas.pyi: - Signature.apply: add Task.apply options - _chain.apply_async, run: add Signature.apply_async options - _basemap.apply_async: add options - chunks.__call__, apply_async: add options - group.__init__: consolidate overloads, add options - group.apply_async: add options - chord.run, apply, apply_async, __call__: add options app/task.pyi: - Task.AsyncResult: add backend, app, parent kwargs - Task.signature, subtask: add Signature options result.pyi: - AsyncResult.collect: add get() options
- main: str (always set in __init__) - steps: defaultdict[str, set[Any]] (always initialized) - user_options: defaultdict[str, set[Any]] (fix type and make non-optional) - builtin_fixups: set[str] (already non-optional) These are always set on instances, so typing them as optional was misleading for users who work with Celery app instances.
- AsyncResult.id: str (always set in __init__) - Task.name: str (always set when task is bound) - Signal.receivers: list (always initialized in __init__) - Celery main, steps, user_options, builtin_fixups (always set in __init__) All attributes are None at class level but typed as non-optional since users work with instances, not the class itself.
Control.inspect is a cached_property that returns a CLASS (type[Inspect]), not an instance. Usage is: app.control.inspect(destination=['worker1'])
| matcher: Callable[..., Any] | None = ..., | ||
| ) -> Inspect: ... | ||
| @property | ||
| def inspect(self) -> type[Inspect]: ... |
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.
The inspect() change above looks a bit weird but is technically correct and not changing DX behavior. Basically Inspect.init() is just being mapped here.
Both are always resolved to bool from config on task instances, never None. Only None at class level before binding.
- Control.app: Celery (required, __init__ immediately accesses app.conf) - AsyncResult.app: Celery (always resolved via app_or_default()) - AsyncResult.backend: Backend (always resolved via self.app.backend) All are None at class level but always non-None on instances.
The celery.app.beat module doesn't exist at runtime - the correct module is celery.apps.beat (note the 's'). This stub was causing stubtest errors.
|
I think this looks good now from my end. If this gets merged I will submit a PR which introduces stubtest and ty into CI, which I think will be important feedback for these and future changes. |
Basically same as #207.
CI should pass if #207 and #209 are merged before this...