Skip to content

Conversation

spikeninja
Copy link

  • add TaskiqAdminMiddleware to middlewares
  • add aiohttp package

@spikeninja spikeninja marked this pull request as draft June 29, 2025 09:53
@spikeninja spikeninja changed the title feat: add TaskiqAdminMiddleware, add aiohttp package feat: add TaskiqAdminMiddleware and aiohttp package Jun 29, 2025
@spikeninja spikeninja marked this pull request as ready for review July 20, 2025 12:56
@spikeninja spikeninja requested a review from s3rius July 20, 2025 12:56
s3rius
s3rius previously approved these changes Jul 20, 2025
Copy link
Member

@s3rius s3rius left a 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"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@smalyu
Copy link

smalyu commented Sep 12, 2025

Thanks for #478!
Could you please consider emitting task lifecycle events to the broker (Redis/RabbitMQ) and letting the admin service consume them asynchronously (Celery/Flower style), instead of reporting status via HTTP from workers?
That would remove per-task HTTP overhead, avoid coupling to admin availability, and scale better under load.

@spikeninja
Copy link
Author

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)

@spikeninja spikeninja force-pushed the feat/add-taskiq-admin-middleware branch 2 times, most recently from 65597e2 to 448624f Compare October 8, 2025 19:18
@spikeninja
Copy link
Author

@s3rius Review it pls

:param result: result of execution for current task.
"""
await self._spawn_request(
f"/api/tasks/{message.task_id}/executed",
Copy link
Contributor

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?

Copy link
Author

@spikeninja spikeninja Oct 10, 2025

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",
{
Copy link
Contributor

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?

Copy link
Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants