Bogdanp / django_dramatiq

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

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

Open ashleybartlett opened 3 years ago

ashleybartlett commented 3 years ago

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'
Bogdanp commented 3 years ago

@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.

ashleybartlett commented 3 years ago

@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.

Bogdanp commented 3 years ago

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.

ashleybartlett commented 3 years ago

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.

dnmellen commented 3 years ago

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.