django_dramatiq icon indicating copy to clipboard operation
django_dramatiq copied to clipboard

Subclassing DjangoDramatiqConfig fails as initialization happens on module import first not getting to subclass

Open ashleybartlett opened this issue 3 years ago • 5 comments

I'm trying to subclass DjangoDramatiqConfig so I can use the GroupCallbacks middleware. Unfortunately as there is a DjangoDramatiqConfig.initialize() call in the base of the apps.py file which gets called before my one, so it just fails as it doesn't know to pass in the kwargs to the middleware.

Have I missed something?

Or is there a reason why a global initialize() call is used, and not a DjangoDramatiqConfig.ready() which is called only slightly later on completion of initialization importing INSTALLED_APPS? ?

Happy to throw a PR up to make the change. Or move it to a __init__() call. I just don't understand why it's global, with no way to block.

  File "/Users/abartlett/src/relist/backend/relist/dramatiq_app.py", line 1, in <module>
    from django_dramatiq.apps import DjangoDramatiqConfig
  File "/Users/abartlett/Library/Caches/pypoetry/virtualenvs/relist-6ZmwRPKa-py3.8/lib/python3.8/site-packages/django_dramatiq/apps.py", line 123, in <module>
    DjangoDramatiqConfig.initialize()
  File "/Users/abartlett/Library/Caches/pypoetry/virtualenvs/relist-6ZmwRPKa-py3.8/lib/python3.8/site-packages/django_dramatiq/apps.py", line 69, in initialize
    middleware = [
  File "/Users/abartlett/Library/Caches/pypoetry/virtualenvs/relist-6ZmwRPKa-py3.8/lib/python3.8/site-packages/django_dramatiq/apps.py", line 70, in <listcomp>
    load_middleware(path, **cls.get_middleware_kwargs(path))
  File "/Users/abartlett/Library/Caches/pypoetry/virtualenvs/relist-6ZmwRPKa-py3.8/lib/python3.8/site-packages/django_dramatiq/utils.py", line 6, in load_middleware
    return import_string(path_or_obj)(**kwargs)
TypeError: __init__() missing 1 required positional argument: 'rate_limiter_backend'

ashleybartlett avatar May 04 '21 23:05 ashleybartlett

@ashleybartlett can you give #103 a try? As far as I can tell, ready should work fine and it does in a test app I created.

Bogdanp avatar May 06 '21 06:05 Bogdanp

@Bogdanp I gave it another try and it works for me.

Initially I hit the same error as when I tried to spike this. This time I spent a few more minutes looking at the exception output. Turns out my admin import was causing some task modules to be imported early, which was causing it to register them before django dramatic had configured the broker, I was getting the pika is not installed error, which it turns out is a red herring.

So I would say this could be a breaking change based on this.

ashleybartlett avatar May 06 '21 10:05 ashleybartlett

Turns out my admin import was causing some task modules to be imported early

Yeah, that sounds familiar. I think that may have been the reason for why the config was initialized so soon. I think this might be worth breaking backwards-compatibility for, but I'll have to play around with it more first.

Bogdanp avatar May 07 '21 06:05 Bogdanp

Yeah, that sounds familiar. I think that may have been the reason for why the config was initialized so soon. I think this might be worth breaking backwards-compatibility for, but I'll have to play around with it more first.

I think it's worth it, I just wanted to call it out.

ashleybartlett avatar May 21 '21 03:05 ashleybartlett

Hi, I'm having some issues related to this. Subclassing the Django configuration was difficult because of the initialize method. Still, although #103 and having ready() is the best way to do it in Django, if you try to import any task from any Django model, you will encounter some errors, especially if you are using middlewares because they are not loaded yet into dramatiq.

  File "/app/core/tasks.py", line 35, in <module>
    def send_email_template(
  File "/venv/lib/python3.8/site-packages/dramatiq/actor.py", line 213, in decorator
    raise ValueError((
ValueError: The following actor options are undefined: store_results. Did you forget to add a middleware to your Broker?

The error above happens because Django imports app models before running ready(), so I managed to run ready at the import models stage. This way, when Django loads the rest of your apps, dramatiq will be fully configured.

from django_dramatiq.apps import DjangoDramatiqConfig


class CustomDjangoDramatiqConfig(DjangoDramatiqConfig):
    def import_models(self):
        super().import_models()
        self.ready()

    def ready(self):
        if not getattr(self, "is_ready", False):
            super().ready()
        self.is_ready = True

    @classmethod
    def middleware_groupcallbacks_kwargs(cls):
        return {"rate_limiter_backend": cls.get_rate_limiter_backend()}

@Bogdanp What do you think about this fix? Could we move the workaround into DjangoDramatiqConfig class? I could make a PR if you're interested.

dnmellen avatar Nov 19 '21 11:11 dnmellen