Expand Settings to handle different Django Architectures #62

itsthejoker · 2022-06-24T18:34:19Z

Expanding from #61

The settings for django-lightweight-queue expect and require that all settings are laid out in a single file and are all ready at runtime, which is not an accurate assumption for projects that use environment variables to determine which settings to load at runtime. This PR refactors the internal project settings to be similar to that of django-cors-headers, though this one is more complex because django-lightweight-queue expects to be able to rewrite every setting on its own.

This is a stacked PR with #64 and this one should be merged first.

PeterJCLaw

Thanks for putting this together and breaking this out. I can definitely see why this would be a useful change 😄 (at the very least it would make override_settings work properly, which would simplify the tests!).

Unfortunately I have a concern about the semantics of the settings overriding -- see the inline comment about what happens if one of the settings is overridden to a falsey value.
I've included comments from a first pass at the whole PR here, however I suggest we work out what the solution to the overriding is before addressing the other comments as any refactor could affect whether/how they apply.

README.md

pyproject.toml

PeterJCLaw · 2022-06-24T18:55:14Z

tests/test_pause_resume.py

    @mock.patch(
-        'django_lightweight_queue.app_settings.BACKEND',
+        'django_lightweight_queue.app_settings.Settings.BACKEND',
        new='django_lightweight_queue.backends.redis.RedisBackend',
    )


Now that the settings are dynamically loaded, what would you think about moving these to using Django's override_settings decorator rather than patching the settings?

For this test, swapping to

@override_settings( LIGHTWEIGHT_QUEUE_BACKEND='django_lightweight_queue.backends.redis.RedisBackend' ) def test_pause_resume(self) -> None: ...

...lets this test pass, but it reports that test_pause_resume_unsupported_backend (the immediate next test) fails. I have essentially zero experience with unittest (all of my personal and professional projects use pytest) -- do you know why this error could be happening?

The newly failing test returns the following, but I'm not sure why:

Traceback (most recent call last): File "/home/joe/code/django-lightweight-queue/tests/test_pause_resume.py", line 87, in test_pause_resume_unsupported_backend with self.assertRaises(CommandError): AssertionError: CommandError not raised

Ah, hrm, I wonder if we need to change the test case to inherit from one of Django's base test classes in order to use override_settings. Haven't tested it, but that might be worth a try. (I believe there is a base class which doesn't try to do any database stuff, which is likely the one we'd want if we do go that direction).

tests/test_reliable_redis_backend.py

django_lightweight_queue/app_settings.py

itsthejoker · 2022-07-10T21:51:51Z

@PeterJCLaw I've refactored it to flesh out your suggestion, but I'm a little concerned that the readability has gone down. (Very pleased about the decrease in length, though.) Ready for review again.

PeterJCLaw

Thanks for making those changes. It definitely turned out a bit more complex than I'd imagined, which doesn't feel great for settings which one would hope ought to be simple! At least the complexity is in one place though, which I think makes up for it.

I think this is nearly there though I'm concerned that we've lost the type checking around the usage of the settings (mypy can't really see through the __getattr__ usage), which I think would be great to keep if possible.

django_lightweight_queue/app_settings.py

PeterJCLaw · 2022-07-13T18:07:50Z

django_lightweight_queue/utils.py

+
+    class Overrides(Settings):
+        """Container for holding internally overridden settings."""
+        pass
+


For my understanding: what does this provide over using Settings() directly?

Two reasons: it makes mypy not throw a fit and since we search by getattr, starting with an empty class means that we can add whatever settings we want and if the getattr fails then it'll fall through to the next layer.

(There was originally a different comment here but it took me a minute to remember why I did it this way.)

We need the _uses_short_names key from the base class so that we look up the right things, and I didn't want to duplicate it in case it ever needed to change 🤷

PeterJCLaw · 2022-07-13T18:14:06Z

django_lightweight_queue/app_settings.py

+    WORKERS: Dict[QueueName, int] = {}
+    BACKEND: str = 'django_lightweight_queue.backends.synchronous.SynchronousBackend'
+    LOGGER_FACTORY: Union[str, Callable[[str], Logger]] = 'logging.getLogger'
+    # Allow per-queue overrides of the backend.
+    BACKEND_OVERRIDES: Dict[QueueName, str] = {}
+    MIDDLEWARE: Sequence[str] = ('django_lightweight_queue.middleware.logging.LoggingMiddleware',)
+    # Apps to ignore when looking for tasks. Apps must be specified as the dotted
+    # name used in `INSTALLED_APPS`. This is expected to be useful when you need to
+    # have a file called `tasks.py` within an app, but don't want
+    # django-lightweight-queue to import that file.
+    # Note: this _doesn't_ prevent tasks being registered from these apps.
+    IGNORE_APPS: Sequence[str] = ()
+    REDIS_HOST: str = '127.0.0.1'
+    REDIS_PORT: int = 6379
+    REDIS_PASSWORD: Optional[str] = None
+    REDIS_PREFIX: str = ""
+    ENABLE_PROMETHEUS: bool = False
+    # Workers will export metrics on this port, and ports following it
+    PROMETHEUS_START_PORT: int = 9300
+    ATOMIC_JOBS: bool = True


Could we include these members, without values, on the base Settings type?
Combined with ensuring that the app_settings variable is typed as Settings I think would that keep type-checking at the places in the code where the settings get used, which I think we do want.

I realise we might then need to typing.cast the assignment of AppSettings, but I think that's ok if so.
I'm guessing that making Settings a protocol didn't work for some reason? (I'd assumed that would avoid the need for a cast in the assignment to app_settings, but perhaps it doesn't?)

Aaah, trying this a bit myself I can see that if Settings is a protocol mypy doesn't like the Overrides class not having the members. If that's the blocker I think the fix is probably to type: ignore it in that one place rather than sacrificing the settings type checking throughout.

(I thiiink there might also be a route involving a LongNameAdapter (see other comment) which solves the typing more completely, though that's possibly a lot more involved as a change so happy to leave that for now if there's a simpler path)

Honestly most of this silliness is related to mypy -- if I had a quarter for each time I've googled something working on this repo so far and found an open bug ticket on the mypy repo, I'd have a very nice dinner out for myself.

I'm fine with ignoring or omitting types wherever needed as long as the values themselves are clear, but it's not my repo so it's whatever's acceptable by you all.

tests/test_pause_resume.py

Co-authored-by: Peter Law <[email protected]>

itsthejoker · 2022-07-13T20:40:17Z

Re: type checking: I admit that I have no idea how to properly pass the type hints through that __getattr__ call -- we passed the limit of my skills appeasing mypy long ago in this process. I'm open to trying whatever, but if you have an idea how to make it work then would you be open to implementing it? At this point I've spent about as much time fighting mypy errors as I have actually writing code and it's quite frustrating.

PeterJCLaw · 2022-07-14T08:55:40Z

Re: type checking: I admit that I have no idea how to properly pass the type hints through that __getattr__ call -- we passed the limit of my skills appeasing mypy long ago in this process. I'm open to trying whatever, but if you have an idea how to make it work then would you be open to implementing it? At this point I've spent about as much time fighting mypy errors as I have actually writing code and it's quite frustrating.

Sure, I'm happy to have a go at this. I realise that mypy errors can be a little obtuse at times! Thanks for your efforts getting us this far :)

PeterJCLaw · 2022-08-02T15:19:47Z

Quick update -- I haven't forgotten this, just haven't had time recently.

PeterJCLaw-mns · 2023-12-14T14:51:22Z

Thanks for building this & sorry it's taken so long to reach a conclusion here. We've ended up extending this in #70 which I've now merged. Hopefully that resolves the use-cases you had in mind.

itsthejoker added 3 commits June 24, 2022 14:18

resolve conflicts

8d396f2

make all settings changeable at runtime

c00635f

make all settings changeable at runtime, try 2

3b9e3a6

itsthejoker mentioned this pull request Jun 24, 2022

Fixes for Debug Web #64

Open

PeterJCLaw requested changes Jun 24, 2022

View reviewed changes

itsthejoker added 10 commits June 24, 2022 19:17

fix missing type hint and revert name to app_settings

737f80d

revert version number update

0b202ac

run isort

97b36d4

move readme changes to own PR

76631b4

appease mypy with appropriate blood-based rituals

73cfefe

revert mock to use .patch.dict()

6ee7218

Refactor for layer approach

c9c5b84

add type hints for getattr

fc48298

fix support for old python

ecbcad3

refactor shortening correctly

05ebae5

re-add accidentally-dropped comment

d29f36a

PeterJCLaw reviewed Jul 13, 2022

View reviewed changes

itsthejoker and others added 2 commits July 13, 2022 16:08

Update django_lightweight_queue/app_settings.py

af4eb29

Co-authored-by: Peter Law <[email protected]>

Update django_lightweight_queue/app_settings.py

496828a

Co-authored-by: Peter Law <[email protected]>

PeterJCLaw mentioned this pull request Aug 6, 2022

Allow runtime settings overrides #70

Merged

itsthejoker mentioned this pull request Dec 9, 2022

docker localdev AlexandriaILS/Alexandria#51

Open

6 tasks

PeterJCLaw-mns closed this Dec 14, 2023

Expand Settings to handle different Django Architectures #62

Expand Settings to handle different Django Architectures #62

Uh oh!

Conversation

itsthejoker commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterJCLaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsthejoker Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

itsthejoker commented Jul 10, 2022

Uh oh!

PeterJCLaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itsthejoker commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterJCLaw commented Jul 14, 2022

Uh oh!

PeterJCLaw commented Aug 2, 2022

Uh oh!

PeterJCLaw-mns commented Dec 14, 2023

Uh oh!

Uh oh!

itsthejoker commented Jun 24, 2022 •

edited

Loading

itsthejoker Jun 25, 2022 •

edited

Loading

itsthejoker commented Jul 13, 2022 •

edited

Loading