-
-
Notifications
You must be signed in to change notification settings - Fork 91
feat: add TaskiqAdminMiddleware and aiohttp package #478
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: master
Are you sure you want to change the base?
Conversation
spikeninja
commented
Jun 29, 2025
- add TaskiqAdminMiddleware to middlewares
- add aiohttp package
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.
I'm not sure if this will work as you intended.
I think it would be better to make all method implementations asynchronous to ensure that the loop is running. While we are pretty much confident that it should execute correctly, it might be somewhat controversial for users who want to test this middleware or use it manually for some reason.
Since TaskiqMiddleware
allows you to define both synchronous and asynchronous methods, you can achieve this by simply adding the async
keyword right before the def
.
What do you think? Does this make sense to you?
pyproject.toml
Outdated
msgpack = { version = "^1.0.7", optional = true } | ||
cbor2 = { version = "^5", optional = true } | ||
izulu = "0.50.0" | ||
aiohttp = "^3.12.13" |
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.
aiohttp = "^3.12.13" | |
aiohttp = "^3" |
Let's unpin this version, to have less strict dependency requirements.
Current one basically translates to >=3.12.13,<3.13.0
. The new one makes it more flexible >=3.0.0,<4.0.0
. It will be easier to integrate in existing projects that use aiohttp.
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.
Done
Thanks for #478! |
Hey @smalyu Yes, I would like to add it but as a separate middleware, smth like TaskqAdminBrokerMiddleware Let me firstly finish refining the http one pls and we can jump into the broker one) |
65597e2
to
448624f
Compare
@s3rius Review it pls |
:param result: result of execution for current task. | ||
""" | ||
await self._spawn_request( | ||
f"/api/tasks/{message.task_id}/executed", |
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.
Maybe parameterize the URI endpoint template so that it can be passed when initializing the middleware? Just in case someone overrides the routing?
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.
No, it's pre-defined on the taskiq-admin side and cannot be edited anyway (I mean the API path: "/api/tasks/.../...")
""" | ||
await self._spawn_request( | ||
f"/api/tasks/{message.task_id}/executed", | ||
{ |
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.
Wouldn’t it be better to define a msgpack schema to strictly specify the interface?
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.
Will add the schema in the future when it becomes possible to add custom fields. Now it's not needed)