Bogdanp / django_dramatiq

A Django app that integrates with Dramatiq.
https://dramatiq.io
Other
331 stars 77 forks source link

Can't use GroupCallbacks middleware #82

Closed dnmellen closed 3 years ago

dnmellen commented 3 years ago

Hi, I'm trying to use GroupCallbacks in django_dramatiq but it seems that callback is not really compatible:

My settings:

if TESTING:
    DRAMATIQ_BROKER = {
        "BROKER": "dramatiq.brokers.stub.StubBroker",
        "OPTIONS": {},
        "MIDDLEWARE": [
            "dramatiq.middleware.AgeLimit",
            "dramatiq.middleware.TimeLimit",
            "dramatiq.middleware.GroupCallbacks",
            "dramatiq.middleware.Callbacks",
            "dramatiq.middleware.Pipelines",
            "dramatiq.middleware.Retries",
            "django_dramatiq.middleware.DbConnectionsMiddleware",
        ]
    }

Error:

File "/venv/lib/python3.7/site-packages/django_dramatiq/apps.py", line 107, in <module>
    DjangoDramatiqConfig.initialize()
  File "/venv/lib/python3.7/site-packages/django_dramatiq/apps.py", line 69, in initialize
    middleware = [load_middleware(path) for path in broker_settings.get("MIDDLEWARE", [])]
  File "/venv/lib/python3.7/site-packages/django_dramatiq/apps.py", line 69, in <listcomp>
    middleware = [load_middleware(path) for path in broker_settings.get("MIDDLEWARE", [])]
  File "/venv/lib/python3.7/site-packages/django_dramatiq/utils.py", line 17, in load_middleware
    return load_class(path_or_obj)()
TypeError: __init__() missing 1 required positional argument: 'rate_limiter_backend'

GroupCallbacks initializer needs rate_limiter_backend as parameter:

class GroupCallbacks(Middleware):
    def __init__(self, rate_limiter_backend):
        self.rate_limiter_backend = rate_limiter_backend

But django_dramatiq is not passing extra params when loading the middleware:

apps.py:

middleware = [load_middleware(path) for path in broker_settings.get("MIDDLEWARE", [])]

utils.py:

def load_middleware(path_or_obj):
    if isinstance(path_or_obj, str):
        return import_string(path_or_obj)()  <-- No param passed
    return path_or_obj

How can I use GroupCallbacks in django_dramatiq? Do I need to modify how django_dramatiq initializes or is there another way?

Thanks!

dnmellen commented 3 years ago

I know that I can pass an instance of the middleware in the settings (ie: TimeLimit middleware)

DRAMATIQ_BROKER = {
    "BROKER": "dramatiq.brokers.stub.StubBroker",
    "OPTIONS": {},
    "MIDDLEWARE": [
        "dramatiq.middleware.AgeLimit",
        TimeLimit(time_limit=36000000),  <-------
        "dramatiq.middleware.Retries",
        "django_dramatiq.middleware.AdminMiddleware",
        "django_dramatiq.middleware.DbConnectionsMiddleware",
    ]
}

But GroupCallbacks middleware needs the rate_limiter_backend which is defined on the django_dramatiq app initialization, so I'm not sure if I can access it in my settings.py

Bogdanp commented 3 years ago

Yes, this is a problem right now. We could come up with some sort of protocol for referring to arguments from a settings.py middleware def, but I'm not sure what would be the common/idiomatic way to do something like this with Django settings. Suggestions would be appreciated.

dnmellen commented 3 years ago

I'm implementing a possible solution in this PR: https://github.com/Bogdanp/django_dramatiq/pull/83, but I'm still fighting with the tests.

dnmellen commented 3 years ago

Please check the PR #83. I couldn't add tests and anyone is welcome to modify it and add some tests (I had a hard time trying to modify the base test settings since I needed to change INSTALLED_APPS settings and it won't work using the override_settings decorator.

Now the PR does not add any tests but the code I added doesn't modify the current behavior of django_dramatiq. I'm going to test my branch with my project. Consider this as a possible solution to this issue.

Bogdanp commented 3 years ago

Closing since the associated PR has been merged. Thanks again!