Skip to content
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

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

Open
ashleybartlett opened this issue May 4, 2021 · 5 comments

Comments

@ashleybartlett
Copy link

ashleybartlett commented May 4, 2021

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 ashleybartlett changed the title Subclassing django_dramatiq.apps.DjangoDramatiqConfig triggers initialization early Subclassing DjangoDramatiqConfig fails as initialization happens on module import first not getting to subclass May 5, 2021
@Bogdanp
Copy link
Owner

Bogdanp commented May 6, 2021

@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
Copy link
Author

@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
Copy link
Owner

Bogdanp commented May 7, 2021

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
Copy link
Author

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
Copy link

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.

Bogdanp pushed a commit that referenced this issue Nov 5, 2022
This commit itself will break test suite, so we can later address issue in #123.
This bug was introduced in #103 where django_dramatiq started benefitting from `AppConfig.ready()` functionality.
There was a research done in #100 explaining the underlying issue.
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

No branches or pull requests

3 participants